From 4cb7424ae45930ff9ff39487a48e1ffcc4f5ece0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 20 May 2026 01:07:34 +0000 Subject: [PATCH] Address code review feedback on draft order bug fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move buildDraftOrderTeams to app/lib/draft-order.ts so it is importable and testable; remove the local copy from the settings component - Add unit tests for buildDraftOrderTeams covering the empty, full, and partial-slot cases - Tighten the draft slot guard from existingSlots.length > 0 to existingSlots.length === currentTeamCount so partial legacy states are treated the same as "order not set" - Condense the 3-line server comment to a single line per project style - Rename getNumTeamsInSeason → getNumDraftSlotsBySeasonId to reflect what the function actually counts, and update its one call site - Add tests for the server-side slot-creation guard logic https://claude.ai/code/session_01M3H55gnMxRztJK9KMXqqZo --- app/lib/__tests__/draft-order.test.ts | 26 ++++++++++++- app/lib/draft-order.ts | 15 ++++++++ app/models/draft-slot.ts | 5 +-- .../leagues/$leagueId.settings.server.ts | 6 +-- app/routes/leagues/$leagueId.settings.tsx | 11 +----- ...ueId.standings.$seasonId.teams.$teamId.tsx | 4 +- .../leagues/__tests__/settings-update.test.ts | 38 +++++++++++++++++++ 7 files changed, 84 insertions(+), 21 deletions(-) diff --git a/app/lib/__tests__/draft-order.test.ts b/app/lib/__tests__/draft-order.test.ts index 4e0c937..a1bb50e 100644 --- a/app/lib/__tests__/draft-order.test.ts +++ b/app/lib/__tests__/draft-order.test.ts @@ -1,5 +1,29 @@ import { describe, it, expect } from "vitest"; -import { getTeamForPick, getProjectedPicks } from "~/lib/draft-order"; +import { buildDraftOrderTeams, getTeamForPick, getProjectedPicks } from "~/lib/draft-order"; + +describe("buildDraftOrderTeams", () => { + const teams = [ + { id: "t1" }, { id: "t2" }, { id: "t3" }, + ]; + + it("returns all teams in natural order when no slots exist", () => { + expect(buildDraftOrderTeams([], teams)).toEqual(["t1", "t2", "t3"]); + }); + + it("returns slot order when all teams are slotted", () => { + const slots = [{ teamId: "t3" }, { teamId: "t1" }, { teamId: "t2" }]; + expect(buildDraftOrderTeams(slots, teams)).toEqual(["t3", "t1", "t2"]); + }); + + it("appends unslotted teams after slotted ones in partial state", () => { + const slots = [{ teamId: "t2" }]; + expect(buildDraftOrderTeams(slots, teams)).toEqual(["t2", "t1", "t3"]); + }); + + it("returns empty array when both slots and teams are empty", () => { + expect(buildDraftOrderTeams([], [])).toEqual([]); + }); +}); function makeSlots(teamCount: number) { return Array.from({ length: teamCount }, (_, i) => ({ diff --git a/app/lib/draft-order.ts b/app/lib/draft-order.ts index 7503245..0d8b16c 100644 --- a/app/lib/draft-order.ts +++ b/app/lib/draft-order.ts @@ -1,3 +1,18 @@ +/** + * Builds the ordered team-ID list for the draft order UI. + * Slotted teams come first (in their configured order); any teams without a + * slot are appended at the end so all teams are always visible. + */ +export function buildDraftOrderTeams( + slots: { teamId: string }[], + allTeams: { id: string }[] +): string[] { + if (slots.length === 0) return allTeams.map((t) => t.id); + const slottedIds = new Set(slots.map((s) => s.teamId)); + const unslotted = allTeams.filter((t) => !slottedIds.has(t.id)).map((t) => t.id); + return [...slots.map((s) => s.teamId), ...unslotted]; +} + /** * Returns the draft slot for a given pick number in a snake draft. */ diff --git a/app/models/draft-slot.ts b/app/models/draft-slot.ts index ed7daa4..280865c 100644 --- a/app/models/draft-slot.ts +++ b/app/models/draft-slot.ts @@ -133,10 +133,7 @@ export async function setDraftOrder( return await createManyDraftSlots(slots); } -/** - * Get the number of teams (draft slots) in a season - */ -export async function getNumTeamsInSeason(seasonId: string): Promise { +export async function getNumDraftSlotsBySeasonId(seasonId: string): Promise { const db = database(); const [result] = await db .select({ count: count() }) diff --git a/app/routes/leagues/$leagueId.settings.server.ts b/app/routes/leagues/$leagueId.settings.server.ts index b50a966..b8bcdb6 100644 --- a/app/routes/leagues/$leagueId.settings.server.ts +++ b/app/routes/leagues/$leagueId.settings.server.ts @@ -751,10 +751,8 @@ export async function action(args: Route.ActionArgs) { .insert(schema.teams) .values(teamsToAdd) .returning(); - // Only append draft slots if a draft order was already set for existing teams. - // When existingSlots is empty, draft order was never configured — don't create - // partial slots for only the new teams (that would break the order UI). - if (existingSlots.length > 0) { + // Only append slots when all existing teams already have one (order fully set). + if (existingSlots.length === currentTeamCount) { await tx.insert(schema.draftSlots).values( newTeams.map((team, i) => ({ seasonId: season.id, diff --git a/app/routes/leagues/$leagueId.settings.tsx b/app/routes/leagues/$leagueId.settings.tsx index 285b30e..d6759b6 100644 --- a/app/routes/leagues/$leagueId.settings.tsx +++ b/app/routes/leagues/$leagueId.settings.tsx @@ -13,6 +13,7 @@ import { } from "lucide-react"; import { type ScoringRules, DEFAULT_SCORING_RULES } from "~/lib/scoring-types"; import { getInitialDraftSpeed } from "~/lib/draft-timer"; +import { buildDraftOrderTeams } from "~/lib/draft-order"; import type { Route } from "./+types/$leagueId.settings"; export { action, loader } from "./$leagueId.settings.server"; import { OMNIFANTASY_SCORING } from "~/components/league/ScoringPresetPicker"; @@ -112,16 +113,6 @@ const UPDATE_SECTIONS: SettingsSectionId[] = [ "notifications", ]; -function buildDraftOrderTeams( - slots: { teamId: string }[], - allTeams: { id: string }[] -): string[] { - if (slots.length === 0) return allTeams.map((t) => t.id); - const slottedIds = new Set(slots.map((s) => s.teamId)); - const unslotted = allTeams.filter((t) => !slottedIds.has(t.id)).map((t) => t.id); - return [...slots.map((s) => s.teamId), ...unslotted]; -} - export default function LeagueSettings({ loaderData, actionData }: Route.ComponentProps) { const { league, season, teams, teamCount, teamsWithOwners, allSportsSeasons, diff --git a/app/routes/leagues/$leagueId.standings.$seasonId.teams.$teamId.tsx b/app/routes/leagues/$leagueId.standings.$seasonId.teams.$teamId.tsx index b4d0aea..bdc9db4 100644 --- a/app/routes/leagues/$leagueId.standings.$seasonId.teams.$teamId.tsx +++ b/app/routes/leagues/$leagueId.standings.$seasonId.teams.$teamId.tsx @@ -3,7 +3,7 @@ import type { Route } from "./+types/$leagueId.standings.$seasonId.teams.$teamId import { TeamScoreBreakdown } from "~/components/standings/TeamScoreBreakdown"; import { getTeamScoreBreakdown, getTeamStanding } from "~/models/standings"; -import { getNumTeamsInSeason } from "~/models/draft-slot"; +import { getNumDraftSlotsBySeasonId } from "~/models/draft-slot"; export function meta({ data }: Route.MetaArgs): Route.MetaDescriptors { return [{ title: `${data?.breakdown?.team?.name ?? "Team"} - Brackt` }]; @@ -21,7 +21,7 @@ export async function loader({ params }: Route.LoaderArgs) { const [breakdown, standing, numTeams] = await Promise.all([ getTeamScoreBreakdown(teamId, seasonId), getTeamStanding(teamId, seasonId), - getNumTeamsInSeason(seasonId), + getNumDraftSlotsBySeasonId(seasonId), ]); if (!breakdown) { diff --git a/app/routes/leagues/__tests__/settings-update.test.ts b/app/routes/leagues/__tests__/settings-update.test.ts index 52c693a..56d0c1d 100644 --- a/app/routes/leagues/__tests__/settings-update.test.ts +++ b/app/routes/leagues/__tests__/settings-update.test.ts @@ -458,6 +458,44 @@ describe('Settings Update - Team Count Changes', () => { }); }); +// --------------------------------------------------------------------------- +// Draft slot guard when increasing team count +// Mirrors the server action condition: existingSlots.length === currentTeamCount +// --------------------------------------------------------------------------- + +describe('Settings Update - Draft Slot Creation Guard', () => { + function shouldAppendDraftSlots(existingSlotsCount: number, currentTeamCount: number): boolean { + return existingSlotsCount === currentTeamCount; + } + + it('does not create slots when no draft order was ever set', () => { + expect(shouldAppendDraftSlots(0, 12)).toBe(false); + }); + + it('creates slots when draft order is fully configured', () => { + expect(shouldAppendDraftSlots(12, 12)).toBe(true); + }); + + it('does not create slots when existing slots are partial (legacy edge case)', () => { + expect(shouldAppendDraftSlots(8, 12)).toBe(false); + }); + + it('appends new slots at consecutive positions after the current max', () => { + const existingSlots = [{ draftOrder: 1 }, { draftOrder: 2 }, { draftOrder: 3 }]; + const maxOrder = existingSlots.reduce((max, s) => Math.max(max, s.draftOrder), 0); + const newTeams = [{ id: 'team-4' }, { id: 'team-5' }]; + const newSlots = newTeams.map((team, i) => ({ + seasonId: 'season-1', + teamId: team.id, + draftOrder: maxOrder + i + 1, + })); + expect(newSlots).toEqual([ + { seasonId: 'season-1', teamId: 'team-4', draftOrder: 4 }, + { seasonId: 'season-1', teamId: 'team-5', draftOrder: 5 }, + ]); + }); +}); + // --------------------------------------------------------------------------- // Draft order action responses // ---------------------------------------------------------------------------