feat(client): add match history & statistics dashboard - #2252
feat(client): add match history & statistics dashboard#2252claytonlin1110 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive local Match History feature to the game client, including IndexedDB persistence, a dedicated history page with filtering, sorting, CSV export, and detailed statistics visualization (such as color win rates, format breakdowns, turn distribution, and per-deck stats). The review feedback highlights two critical issues: a duplicate write bug in React StrictMode due to asynchronous state updates in a useEffect hook (recommending a useRef instead), and the use of unkeyed fragments inside a map loop in the deck statistics table, which can trigger React rendering warnings and bugs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const [historyRecorded, setHistoryRecorded] = useState(false); | ||
| useEffect(() => { | ||
| if (historyRecorded || mode === "spectate") return; | ||
|
|
||
| const outcome = isDraw ? "draw" : isVictory ? "win" : "loss"; | ||
| const formatParam = searchParams.get("format") ?? "Standard"; | ||
| const deckNameFromStorage = localStorage.getItem("phase-active-deck"); | ||
| const feedColors = deckNameFromStorage ? getDeckColorIdentity(deckNameFromStorage) : []; | ||
| const deckColors = parseColorIdentity(feedColors); | ||
| const commanderName: string | null = null; | ||
|
|
||
| const safeMode: MatchMode = ( | ||
| mode === "ai" || mode === "local" || mode === "online" || | ||
| mode === "p2p-host" || mode === "p2p-join" || mode === "draft-match" | ||
| ) ? mode as MatchMode : "ai"; | ||
|
|
||
| void useMatchHistoryStore.getState().addRecord({ | ||
| id: crypto.randomUUID(), | ||
| startedAt: gameStartedAt ?? (Date.now() - (turnCount * 90_000)), | ||
| endedAt: Date.now(), | ||
| format: formatParam, | ||
| mode: safeMode, | ||
| outcome, | ||
| turnCount, | ||
| playerLife, | ||
| opponentLife, | ||
| playerCount: players?.length ?? 2, | ||
| deckName: deckNameFromStorage, | ||
| deckColors, | ||
| aiDifficulty: safeMode === "ai" ? difficulty : undefined, | ||
| commanderName, | ||
| }); | ||
| setHistoryRecorded(true); | ||
| // Run once on mount — all dependencies are stable captures at game-end time. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
In React 18+ StrictMode, useEffect hooks with empty dependency arrays run twice on mount in development. Since state updates are asynchronous, both runs of the effect will see historyRecorded as false and trigger duplicate writes to IndexedDB, corrupting the local match history.
Using a useRef instead of useState ensures synchronous updates that persist across StrictMode's double-render.
Additionally, gameStartedAt is currently only set in handleWsEvent (which is only called for online/spectate modes), meaning it remains null for AI, local, and P2P modes. This causes the start time to always fall back to an estimate and breaks the game duration statistics on the game over screen. Consider initializing gameStartedAt in GamePage when gameState is first loaded.
const historyRecorded = useRef(false);
useEffect(() => {
if (historyRecorded.current || mode === "spectate") return;
historyRecorded.current = true;
const outcome = isDraw ? "draw" : isVictory ? "win" : "loss";
const formatParam = searchParams.get("format") ?? "Standard";
const deckNameFromStorage = localStorage.getItem("phase-active-deck");
const feedColors = deckNameFromStorage ? getDeckColorIdentity(deckNameFromStorage) : [];
const deckColors = parseColorIdentity(feedColors);
const commanderName: string | null = null;
const safeMode: MatchMode = (
mode === "ai" || mode === "local" || mode === "online" ||
mode === "p2p-host" || mode === "p2p-join" || mode === "draft-match"
) ? mode as MatchMode : "ai";
void useMatchHistoryStore.getState().addRecord({
id: crypto.randomUUID(),
startedAt: gameStartedAt ?? (Date.now() - (turnCount * 90_000)),
endedAt: Date.now(),
format: formatParam,
mode: safeMode,
outcome,
turnCount,
playerLife,
opponentLife,
playerCount: players?.length ?? 2,
deckName: deckNameFromStorage,
deckColors,
aiDifficulty: safeMode === "ai" ? difficulty : undefined,
commanderName,
});
// Run once on mount — all dependencies are stable captures at game-end time.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
| {sorted.map((deck) => ( | ||
| <> | ||
| <tr | ||
| key={deck.deckName} | ||
| className="cursor-pointer transition-colors hover:bg-slate-800/30" | ||
| onClick={() => setExpanded(expanded === deck.deckName ? null : deck.deckName)} | ||
| > | ||
| <td className="px-4 py-2.5"> | ||
| <div className="flex items-center gap-2"> | ||
| {deck.colors.length > 0 && ( | ||
| <span className="flex shrink-0 items-center gap-0.5"> | ||
| {deck.colors.map((c) => ( | ||
| <ManaSymbol key={c} shard={c} size="xs" /> | ||
| ))} | ||
| </span> | ||
| )} | ||
| <span className="font-medium text-slate-200">{deck.deckName}</span> | ||
| <span className="rounded bg-slate-700/40 px-1.5 py-0.5 text-xs text-slate-500"> | ||
| {deck.record.total} | ||
| </span> | ||
| </div> | ||
| </td> | ||
| <td className="px-3 py-2.5 text-right"> | ||
| <span className="font-medium text-slate-200"> | ||
| {deck.record.wins}W / {deck.record.losses}L | ||
| {deck.record.draws > 0 ? ` / ${deck.record.draws}D` : ""} | ||
| </span> | ||
| </td> | ||
| <td className="px-3 py-2.5 text-right"> | ||
| <span className={`font-semibold tabular-nums ${ | ||
| deck.record.winRate >= 0.6 | ||
| ? "text-emerald-400" | ||
| : deck.record.winRate < 0.4 | ||
| ? "text-red-400" | ||
| : "text-slate-200" | ||
| }`}> | ||
| {Math.round(deck.record.winRate * 100)}% | ||
| </span> | ||
| </td> | ||
| <td className="px-3 py-2.5 text-right tabular-nums text-slate-400"> | ||
| {deck.avgTurns > 0 ? deck.avgTurns.toFixed(1) : "—"} | ||
| </td> | ||
| <td className="px-3 py-2.5 text-right text-slate-500"> | ||
| {formatDate(deck.lastPlayedAt)} | ||
| </td> | ||
| </tr> | ||
| {expanded === deck.deckName && ( | ||
| <tr key={`${deck.deckName}-detail`} className="bg-slate-800/20"> | ||
| <td colSpan={5} className="px-6 py-3"> | ||
| <div className="flex flex-wrap gap-6"> | ||
| <div className="flex flex-col gap-1.5 flex-1 min-w-[200px]"> | ||
| <span className="text-xs text-slate-500">Win rate breakdown</span> | ||
| <WinRateBar | ||
| winRate={deck.record.winRate} | ||
| wins={deck.record.wins} | ||
| losses={deck.record.losses} | ||
| draws={deck.record.draws} | ||
| /> | ||
| <div className="flex gap-3 text-xs"> | ||
| <span className="text-emerald-400">{deck.record.wins} wins</span> | ||
| <span className="text-red-400">{deck.record.losses} losses</span> | ||
| {deck.record.draws > 0 && ( | ||
| <span className="text-slate-400">{deck.record.draws} draws</span> | ||
| )} | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col gap-1"> | ||
| <span className="text-xs text-slate-500">Avg. game duration</span> | ||
| <span className="text-sm text-slate-200"> | ||
| {deck.avgDuration > 0 ? formatDuration(deck.avgDuration) : "—"} | ||
| </span> | ||
| </div> | ||
| <div className="flex flex-col gap-1"> | ||
| <span className="text-xs text-slate-500">Avg. turns</span> | ||
| <span className="text-sm text-slate-200"> | ||
| {deck.avgTurns > 0 ? deck.avgTurns.toFixed(1) : "—"} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </td> | ||
| </tr> | ||
| )} | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
Using an unkeyed fragment (<>) inside a .map() loop will trigger React warnings about missing unique keys and can cause rendering/reordering bugs.
Please replace the unkeyed fragment with a keyed <Fragment key={deck.deckName}> and import Fragment from "react".
{sorted.map((deck) => (
<Fragment key={deck.deckName}>
<tr
className="cursor-pointer transition-colors hover:bg-slate-800/30"
onClick={() => setExpanded(expanded === deck.deckName ? null : deck.deckName)}
>
<td className="px-4 py-2.5">
<div className="flex items-center gap-2">
{deck.colors.length > 0 && (
<span className="flex shrink-0 items-center gap-0.5">
{deck.colors.map((c) => (
<ManaSymbol key={c} shard={c} size="xs" />
))}
</span>
)}
<span className="font-medium text-slate-200">{deck.deckName}</span>
<span className="rounded bg-slate-700/40 px-1.5 py-0.5 text-xs text-slate-500">
{deck.record.total}
</span>
</div>
</td>
<td className="px-3 py-2.5 text-right">
<span className="font-medium text-slate-200">
{deck.record.wins}W / {deck.record.losses}L
{deck.record.draws > 0 ? ` / ${deck.record.draws}D` : ""}
</span>
</td>
<td className="px-3 py-2.5 text-right">
<span className={`font-semibold tabular-nums ${
deck.record.winRate >= 0.6
? "text-emerald-400"
: deck.record.winRate < 0.4
? "text-red-400"
: "text-slate-200"
}`}>
{Math.round(deck.record.winRate * 100)}%
</span>
</td>
<td className="px-3 py-2.5 text-right tabular-nums text-slate-400">
{deck.avgTurns > 0 ? deck.avgTurns.toFixed(1) : "—"}
</td>
<td className="px-3 py-2.5 text-right text-slate-500">
{formatDate(deck.lastPlayedAt)}
</td>
</tr>
{expanded === deck.deckName && (
<tr key={`${deck.deckName}-detail`} className="bg-slate-800/20">
<td colSpan={5} className="px-6 py-3">
<div className="flex flex-wrap gap-6">
<div className="flex flex-col gap-1.5 flex-1 min-w-[200px]">
<span className="text-xs text-slate-500">Win rate breakdown</span>
<WinRateBar
winRate={deck.record.winRate}
wins={deck.record.wins}
losses={deck.record.losses}
draws={deck.record.draws}
/>
<div className="flex gap-3 text-xs">
<span className="text-emerald-400">{deck.record.wins} wins</span>
<span className="text-red-400">{deck.record.losses} losses</span>
{deck.record.draws > 0 && (
<span className="text-slate-400">{deck.record.draws} draws</span>
)}
</div>
</div>
<div className="
🤖 Architecture Review (automated)Verdict: Seam: PASS — Pure frontend feature; stats are display-layer aggregation over engine-decided outcomes (the
Findings
|
|
@claytonlin1110 Same note as on #2253, and thank you for this one too. This is a lot of thoughtful work. I'm going to pass here for the same reason: match history and the stats dashboard are big player-facing feature surfaces with a lot of product and UX decisions baked in, and those are calls I want to make and own directly rather than merge in after the fact. It's nothing about the quality of the code. The engine and parser issues remain the best place to land contributions right now. They're well-scoped, have a clear correctness bar, and there's a ton still open. I'd genuinely love to see you pick one up, and I'm happy to work through it with you when you do. Closing this for now. Thank you again, sincerely. |
Summary
completed game records a compact
MatchRecord(outcome, format, mode,deck name, turns, duration, life totals) so history survives page reloads
/historyroute with two tabs:GameOverScreenon every mode (ai, local, online, p2p) except spectateTest plan
/historyunder Games tabpnpm tsc --noEmitpasses with no errors