From 9ae5bd4cf69f59b9938c91790ae21e9675d6efa4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 14 May 2026 22:08:00 +0000 Subject: [PATCH] Address code review feedback on MLS standings + simulator 1. Update mls-simulator.ts module comment to document the new step-3 externalId fallback in the conference resolution order 2. Tighten normalizeConference to exact-match "Eastern"/"Western Conference" instead of broad substring, preventing false matches on names like "Northeast" 3. Pre-parse statsMap for each entry before the sort so it isn't rebuilt O(n log n) times during comparison 4. Document the externalId name-matching tradeoff on parseConferenceFromExternalId 5. Try ESPN's gamesPlayed stat before falling back to wins+losses+ties sum; export parseConferenceFromExternalId for direct testing 6. Add tests: normalizeConference passthrough, winPct=0 at preseason, and parseConferenceFromExternalId (case-insensitivity, null/undefined, numeric ESPN IDs, unrecognized strings) https://claude.ai/code/session_01WhzXHpv6taXdHzhgvnv83u --- .../__tests__/mls-simulator.test.ts | 31 +++++++++ app/services/simulations/mls-simulator.ts | 11 ++- .../standings-sync/__tests__/mls.test.ts | 69 +++++++++++++++++++ app/services/standings-sync/mls.ts | 30 ++++---- 4 files changed, 127 insertions(+), 14 deletions(-) diff --git a/app/services/simulations/__tests__/mls-simulator.test.ts b/app/services/simulations/__tests__/mls-simulator.test.ts index 16df30d..4712515 100644 --- a/app/services/simulations/__tests__/mls-simulator.test.ts +++ b/app/services/simulations/__tests__/mls-simulator.test.ts @@ -5,6 +5,7 @@ import { simulateMlsBestOfThree, simulateMlsRegularSeason, simulateMlsPlayoffs, + parseConferenceFromExternalId, type MlsConferenceSeed, type MlsTeamEntry, } from "../mls-simulator"; @@ -361,3 +362,33 @@ describe("mls_bracket simulator sync", () => { }); }); }); + +// ─── parseConferenceFromExternalId ──────────────────────────────────────────── + +describe("parseConferenceFromExternalId", () => { + it('returns "Eastern" for exact match', () => { + expect(parseConferenceFromExternalId("Eastern")).toBe("Eastern"); + }); + + it('returns "Western" for exact match', () => { + expect(parseConferenceFromExternalId("Western")).toBe("Western"); + }); + + it("is case-insensitive", () => { + expect(parseConferenceFromExternalId("eastern")).toBe("Eastern"); + expect(parseConferenceFromExternalId("WESTERN")).toBe("Western"); + }); + + it("returns null for a numeric ESPN team id", () => { + expect(parseConferenceFromExternalId("339")).toBeNull(); + }); + + it("returns null for null/undefined", () => { + expect(parseConferenceFromExternalId(null)).toBeNull(); + expect(parseConferenceFromExternalId(undefined)).toBeNull(); + }); + + it("returns null for an unrecognized string", () => { + expect(parseConferenceFromExternalId("Northeast")).toBeNull(); + }); +}); diff --git a/app/services/simulations/mls-simulator.ts b/app/services/simulations/mls-simulator.ts index 2e395ba..19f5fee 100644 --- a/app/services/simulations/mls-simulator.ts +++ b/app/services/simulations/mls-simulator.ts @@ -18,7 +18,10 @@ * Conference assignment resolution order: * 1. regularSeasonStandings.conference (populated by standings sync) * 2. seasonParticipantSimulatorInputs.region ("Eastern" or "Western") - * 3. Error — conference must be known for all teams + * 3. seasonParticipant.externalId set to "Eastern" or "Western" (pre-sync bootstrap; + * note: this prevents externalId-based matching in future syncs for that participant, + * which falls back to slower name-matching — acceptable tradeoff for bootstrapping) + * 4. Error — conference must be known for all teams * * MLS Cup Playoffs format (18 teams: 9 East + 9 West): * Wild Card : E8 vs E9, W8 vs W9 — single game (no draws; PKs if tied) @@ -70,8 +73,12 @@ function configNumber(config: Record | undefined, key: string, * Infer MLS conference from seasonParticipant.externalId. * Admins can set externalId to "Eastern" or "Western" before the first standings * sync, mirroring how LLWS uses externalId to encode pool assignment. + * + * Side-effect: once externalId holds a conference name instead of an ESPN team ID, + * the standings-sync write-back won't overwrite it (write-back only fires when + * externalId is null). That participant will always match by name, never by ID. */ -function parseConferenceFromExternalId(externalId: string | null | undefined): MlsConference | null { +export function parseConferenceFromExternalId(externalId: string | null | undefined): MlsConference | null { if (!externalId) return null; const upper = externalId.toUpperCase(); if (upper === "EASTERN") return "Eastern"; diff --git a/app/services/standings-sync/__tests__/mls.test.ts b/app/services/standings-sync/__tests__/mls.test.ts index b6a220c..d0f9965 100644 --- a/app/services/standings-sync/__tests__/mls.test.ts +++ b/app/services/standings-sync/__tests__/mls.test.ts @@ -214,4 +214,73 @@ describe("MlsStandingsAdapter", () => { const adapter = new MlsStandingsAdapter(); await expect(adapter.fetchStandings()).rejects.toThrow("no entries"); }); + + it("passes through unrecognized conference names unchanged", async () => { + vi.mocked(fetch).mockResolvedValueOnce({ + ok: true, + json: async () => ({ + children: [ + { + name: "Southern Division", + standings: { + entries: [ + { + team: { id: "1", displayName: "Test FC" }, + stats: [ + makeStat("wins", 5), + makeStat("losses", 3), + makeStat("ties", 2), + makeStat("points", 17), + makeStat("pointsFor", 12), + makeStat("pointsAgainst", 9), + makeStat("pointDifferential", 3), + makeStat("playoffSeed", 1), + ], + }, + ], + }, + }, + ], + }), + } as Response); + + const adapter = new MlsStandingsAdapter(); + const records = await adapter.fetchStandings(); + expect(records[0].conference).toBe("Southern Division"); + }); + + it("returns winPct of 0 when gamesPlayed is 0", async () => { + vi.mocked(fetch).mockResolvedValueOnce({ + ok: true, + json: async () => ({ + children: [ + { + name: "Eastern Conference", + standings: { + entries: [ + { + team: { id: "1", displayName: "Preseason FC" }, + stats: [ + makeStat("wins", 0), + makeStat("losses", 0), + makeStat("ties", 0), + makeStat("points", 0), + makeStat("pointsFor", 0), + makeStat("pointsAgainst", 0), + makeStat("pointDifferential", 0), + makeStat("playoffSeed", 1), + ], + }, + ], + }, + }, + ], + }), + } as Response); + + const adapter = new MlsStandingsAdapter(); + const records = await adapter.fetchStandings(); + expect(records[0].winPct).toBe(0); + expect(records[0].gamesPlayed).toBe(0); + }); }); diff --git a/app/services/standings-sync/mls.ts b/app/services/standings-sync/mls.ts index 8d87bec..359edef 100644 --- a/app/services/standings-sync/mls.ts +++ b/app/services/standings-sync/mls.ts @@ -71,8 +71,8 @@ function flattenEspnStandings( */ function normalizeConference(raw: string): string { const upper = raw.toUpperCase(); - if (upper.includes("EASTERN") || upper.includes("EAST")) return "Eastern"; - if (upper.includes("WESTERN") || upper.includes("WEST")) return "Western"; + if (upper === "EASTERN" || upper === "EASTERN CONFERENCE") return "Eastern"; + if (upper === "WESTERN" || upper === "WESTERN CONFERENCE") return "Western"; return raw; } @@ -100,23 +100,29 @@ export class MlsStandingsAdapter implements StandingsSyncAdapter { ); } - // Sort by points desc for overall league rank - const sorted = [...flattened].toSorted((a, b) => { - const ptsA = statsMap(a.entry.stats).get("points")?.value ?? 0; - const ptsB = statsMap(b.entry.stats).get("points")?.value ?? 0; + // Pre-parse each entry's stats once, then sort by points desc for overall league rank + const parsed = flattened.map(({ entry, conference }) => ({ + entry, + conference, + sm: statsMap(entry.stats), + })); + + const sorted = [...parsed].toSorted((a, b) => { + const ptsA = a.sm.get("points")?.value ?? 0; + const ptsB = b.sm.get("points")?.value ?? 0; if (ptsB !== ptsA) return ptsB - ptsA; - const gdA = statsMap(a.entry.stats).get("pointDifferential")?.value ?? 0; - const gdB = statsMap(b.entry.stats).get("pointDifferential")?.value ?? 0; + const gdA = a.sm.get("pointDifferential")?.value ?? 0; + const gdB = b.sm.get("pointDifferential")?.value ?? 0; return gdB - gdA; }); - return sorted.map(({ entry, conference }, leagueIdx): FetchedStandingsRecord => { - const sm = statsMap(entry.stats); - + return sorted.map(({ entry, conference, sm }, leagueIdx): FetchedStandingsRecord => { const wins = sm.get("wins")?.value ?? 0; const losses = sm.get("losses")?.value ?? 0; const ties = sm.get("ties")?.value ?? sm.get("draws")?.value ?? 0; - const gamesPlayed = Math.round(wins + losses + ties); + const gamesPlayed = Math.round( + sm.get("gamesPlayed")?.value ?? wins + losses + ties + ); const tablePoints = sm.get("points")?.value; const goalsFor = sm.get("pointsFor")?.value ?? sm.get("gf")?.value;