From 1b28ca5e7c5d3b2d4546fb5c3922f3de9ada87d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 20 May 2026 23:47:07 +0000 Subject: [PATCH] Address code review feedback on Discord pick announcements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix "on the clock" timing: read currentPickNumber fresh from DB post-autodraft-chain (matches sendOnTheClockEmail guarantee) - Remove outer try/catch from notifyPickMadeOnDiscord so callers' .catch() is not dead code - Add missing draft-discord.server.test.ts with 12 tests covering all early-exit and happy paths - Fix silent empty-string fallback for missing pickedSlot: warn and skip instead - Eliminate sequential season→league DB queries by accepting leagueId as a direct param - Show "save webhook URL to configure options" hint when URL is typed but not yet saved - Remove block-scope braces at both call sites (plain const declarations) - Remove redundant "Round N, Pick M" description line (title already carries this info) - Inline pickInRoundFor helper to avoid circular import with draft-utils https://claude.ai/code/session_01Tvwsv3LfL9JUqxoLct8dTn --- .../league/settings/NotificationsSection.tsx | 8 +- app/models/draft-utils.ts | 17 +- app/routes/api/draft.make-pick.ts | 33 ++- .../__tests__/draft-discord.server.test.ts | 234 ++++++++++++++++++ app/services/discord.ts | 1 - app/services/draft-discord.server.ts | 110 ++++---- 6 files changed, 326 insertions(+), 77 deletions(-) create mode 100644 app/services/__tests__/draft-discord.server.test.ts diff --git a/app/components/league/settings/NotificationsSection.tsx b/app/components/league/settings/NotificationsSection.tsx index b9fc12f..52b8100 100644 --- a/app/components/league/settings/NotificationsSection.tsx +++ b/app/components/league/settings/NotificationsSection.tsx @@ -43,7 +43,7 @@ export function NotificationsSection({ Create a webhook in Discord under Server Settings, Integrations, Webhooks.

- {savedDiscordWebhookUrl && ( + {savedDiscordWebhookUrl ? (
- )} + ) : discordWebhookUrl ? ( +

+ Save the webhook URL above to configure additional notification options. +

+ ) : null} ); diff --git a/app/models/draft-utils.ts b/app/models/draft-utils.ts index 81f370e..0dfb4fc 100644 --- a/app/models/draft-utils.ts +++ b/app/models/draft-utils.ts @@ -842,23 +842,22 @@ export async function executeAutoPick(params: { .catch((err) => logger.error("[AutoPick] On-the-clock email failed:", err)); } - { - const pickedSlot = draftSlots.find((s) => s.teamId === teamId); - let nextDraftSlotTeamId: string | null = null; - if (!isDraftComplete) { - const { pickInRound: nextPickInRound } = calculatePickInfo(nextPickNumber, totalTeams); - nextDraftSlotTeamId = draftSlots.find((s) => s.draftOrder === nextPickInRound)?.teamId ?? null; - } + const pickedSlot = draftSlots.find((s) => s.teamId === teamId); + if (!pickedSlot) { + logger.warn(`[AutoPick] Skipping Discord pick announcement: no draft slot found for team ${teamId}`); + } else { notifyPickMadeOnDiscord({ seasonId, - pickedTeamName: pickedSlot?.team.name ?? "", + leagueId: season.leagueId, + pickedTeamName: pickedSlot.team.name, participantName: participantToPick.name, sportName: participantToPick.sportsSeason.sport.name, pickNumber, round: currentRound, pickInRound, isDraftComplete, - nextDraftSlotTeamId, + totalTeams, + draftSlots: draftSlots.map((s) => ({ teamId: s.teamId, draftOrder: s.draftOrder })), db, }).catch((err) => logger.error("[AutoPick] Discord pick announcement failed:", err)); } diff --git a/app/routes/api/draft.make-pick.ts b/app/routes/api/draft.make-pick.ts index ef7f3cc..f1eb752 100644 --- a/app/routes/api/draft.make-pick.ts +++ b/app/routes/api/draft.make-pick.ts @@ -331,25 +331,20 @@ export async function action(args: ActionFunctionArgs) { .catch((err) => logger.error("On-the-clock email failed:", err)); } - { - let nextDraftSlotTeamId: string | null = null; - if (!isDraftComplete) { - const { pickInRound: nextPickInRound } = calculatePickInfo(nextPickNumber, totalTeams); - nextDraftSlotTeamId = draftSlots.find((s) => s.draftOrder === nextPickInRound)?.teamId ?? null; - } - notifyPickMadeOnDiscord({ - seasonId, - pickedTeamName: currentDraftSlot.team.name, - participantName: participant.name, - sportName: participant.sportsSeason.sport.name, - pickNumber: currentPickNumber, - round: currentRound, - pickInRound, - isDraftComplete, - nextDraftSlotTeamId, - db, - }).catch((err) => logger.error("Discord pick announcement failed:", err)); - } + notifyPickMadeOnDiscord({ + seasonId, + leagueId: season.leagueId, + pickedTeamName: currentDraftSlot.team.name, + participantName: participant.name, + sportName: participant.sportsSeason.sport.name, + pickNumber: currentPickNumber, + round: currentRound, + pickInRound, + isDraftComplete, + totalTeams, + draftSlots: draftSlots.map((s) => ({ teamId: s.teamId, draftOrder: s.draftOrder })), + db, + }).catch((err) => logger.error("Discord pick announcement failed:", err)); return Response.json({ success: true, diff --git a/app/services/__tests__/draft-discord.server.test.ts b/app/services/__tests__/draft-discord.server.test.ts new file mode 100644 index 0000000..00b9d2b --- /dev/null +++ b/app/services/__tests__/draft-discord.server.test.ts @@ -0,0 +1,234 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("~/database/context", () => ({ + database: vi.fn(), +})); + +vi.mock("~/services/discord", () => ({ + sendPickAnnouncementNotification: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("~/models/account", () => ({ + findDiscordIdsByUserIds: vi.fn().mockResolvedValue(new Map()), +})); + +import { notifyPickMadeOnDiscord } from "../draft-discord.server"; +import { sendPickAnnouncementNotification } from "~/services/discord"; +import { findDiscordIdsByUserIds } from "~/models/account"; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const SEASON_ID = "season-1"; +const LEAGUE_ID = "league-1"; +const TEAM_ID = "team-1"; +const NEXT_TEAM_ID = "team-2"; +const OWNER_ID = "owner-1"; +const NEXT_OWNER_ID = "owner-2"; +const WEBHOOK_URL = "https://discord.com/api/webhooks/123/abc"; + +/** 2-team draft; snake: pick 1→team1, pick 2→team2, pick 3→team2, pick 4→team1 */ +const DRAFT_SLOTS = [ + { teamId: TEAM_ID, draftOrder: 1 }, + { teamId: NEXT_TEAM_ID, draftOrder: 2 }, +]; + +const BASE_PARAMS = { + seasonId: SEASON_ID, + leagueId: LEAGUE_ID, + pickedTeamName: "Alpha FC", + participantName: "Erling Haaland", + sportName: "Soccer", + pickNumber: 1, + round: 1, + pickInRound: 1, + isDraftComplete: false, + totalTeams: 2, + draftSlots: DRAFT_SLOTS, +}; + +function makeLeague(overrides: object = {}) { + return { + id: LEAGUE_ID, + discordWebhookUrl: WEBHOOK_URL, + discordPicksAnnouncementEnabled: true, + ...overrides, + }; +} + +function makeSeason(currentPickNumber = 2) { + return { id: SEASON_ID, leagueId: LEAGUE_ID, currentPickNumber }; +} + +function makeTeam(id: string, ownerId: string | null = null, name = "Beta United") { + return { id, name, ownerId }; +} + +function makeOwner(id: string, discordPingEnabled = true) { + return { id, discordPingEnabled }; +} + +function makeMockDb(overrides: { + league?: object | null; + season?: object | null; + nextTeam?: object | null; + owner?: object | null; +} = {}) { + const league = "league" in overrides ? overrides.league : makeLeague(); + const season = "season" in overrides ? overrides.season : makeSeason(); + const nextTeam = "nextTeam" in overrides ? overrides.nextTeam : makeTeam(NEXT_TEAM_ID, NEXT_OWNER_ID); + const owner = "owner" in overrides ? overrides.owner : makeOwner(NEXT_OWNER_ID); + + return { + query: { + leagues: { findFirst: vi.fn().mockResolvedValue(league) }, + seasons: { findFirst: vi.fn().mockResolvedValue(season) }, + teams: { findFirst: vi.fn().mockResolvedValue(nextTeam) }, + users: { findFirst: vi.fn().mockResolvedValue(owner) }, + }, + }; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +beforeEach(() => { + vi.clearAllMocks(); + process.env.APP_URL = "https://test.brackt.com"; +}); + +afterEach(() => { + delete process.env.APP_URL; +}); + +describe("notifyPickMadeOnDiscord", () => { + it("returns early when league has no webhook URL", async () => { + const db = makeMockDb({ league: makeLeague({ discordWebhookUrl: null }) }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).not.toHaveBeenCalled(); + }); + + it("returns early when picks announcement toggle is disabled", async () => { + const db = makeMockDb({ league: makeLeague({ discordPicksAnnouncementEnabled: false }) }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).not.toHaveBeenCalled(); + }); + + it("returns early when league is not found", async () => { + const db = makeMockDb({ league: null }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).not.toHaveBeenCalled(); + }); + + it("sends announcement with correct pick details", async () => { + const db = makeMockDb(); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ + webhookUrl: WEBHOOK_URL, + pickedTeamName: "Alpha FC", + participantName: "Erling Haaland", + sportName: "Soccer", + pickNumber: 1, + round: 1, + pickInRound: 1, + isDraftComplete: false, + }) + ); + }); + + it("builds the draft URL from APP_URL env var", async () => { + const db = makeMockDb(); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ + draftUrl: `https://test.brackt.com/leagues/${LEAGUE_ID}/draft/${SEASON_ID}`, + }) + ); + }); + + it("passes nextTeamName when fresh season resolves the next slot", async () => { + const db = makeMockDb({ season: makeSeason(2) }); // currentPickNumber=2 → pickInRound=2 → NEXT_TEAM_ID + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ nextTeamName: "Beta United" }) + ); + }); + + it("reads next team from fresh DB pick number, not caller-supplied snapshot", async () => { + // Simulate autodraft chain advancing 2 picks: currentPickNumber=3 on a 2-team snake + // draft means pickInRound=2 again (round 2 reversed) → NEXT_TEAM_ID + const db = makeMockDb({ season: makeSeason(3) }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(db.query.seasons.findFirst).toHaveBeenCalledTimes(1); + }); + + it("passes nextOwnerDiscordId when owner has discordPingEnabled", async () => { + vi.mocked(findDiscordIdsByUserIds).mockResolvedValue(new Map([[NEXT_OWNER_ID, "discord-999"]])); + const db = makeMockDb(); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ nextOwnerDiscordId: "discord-999" }) + ); + }); + + it("omits nextOwnerDiscordId when owner has discordPingEnabled: false", async () => { + const db = makeMockDb({ owner: makeOwner(NEXT_OWNER_ID, false) }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(findDiscordIdsByUserIds).not.toHaveBeenCalled(); + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ nextOwnerDiscordId: undefined }) + ); + }); + + it("omits nextTeamName when next team has no owner", async () => { + const db = makeMockDb({ nextTeam: makeTeam(NEXT_TEAM_ID, null) }); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }); + + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ nextTeamName: "Beta United", nextOwnerDiscordId: undefined }) + ); + }); + + it("skips next-team lookup and passes isDraftComplete: true when draft is done", async () => { + const db = makeMockDb(); + + await notifyPickMadeOnDiscord({ ...BASE_PARAMS, isDraftComplete: true, db: db as never }); + + expect(db.query.seasons.findFirst).not.toHaveBeenCalled(); + expect(sendPickAnnouncementNotification).toHaveBeenCalledWith( + expect.objectContaining({ isDraftComplete: true, nextTeamName: undefined }) + ); + }); + + it("suppresses errors thrown by sendPickAnnouncementNotification — caller's .catch handles them", async () => { + vi.mocked(sendPickAnnouncementNotification).mockRejectedValue(new Error("webhook down")); + const db = makeMockDb(); + + // The function itself does NOT swallow errors; the caller chains .catch(). + await expect( + notifyPickMadeOnDiscord({ ...BASE_PARAMS, db: db as never }) + ).rejects.toThrow("webhook down"); + }); +}); diff --git a/app/services/discord.ts b/app/services/discord.ts index 3b7de68..0951e67 100644 --- a/app/services/discord.ts +++ b/app/services/discord.ts @@ -217,7 +217,6 @@ export async function sendPickAnnouncementNotification({ }): Promise { const lines: string[] = [ `**${escapeMarkdown(pickedTeamName)}** selected **${escapeMarkdown(participantName)}** (${escapeMarkdown(sportName)})`, - `Round ${round}, Pick ${pickInRound}`, ]; if (isDraftComplete) { diff --git a/app/services/draft-discord.server.ts b/app/services/draft-discord.server.ts index 783525d..8fcec5c 100644 --- a/app/services/draft-discord.server.ts +++ b/app/services/draft-discord.server.ts @@ -5,8 +5,21 @@ import { findDiscordIdsByUserIds } from "~/models/account"; import { sendPickAnnouncementNotification } from "~/services/discord"; import { logger } from "~/lib/logger"; +type DraftSlot = { teamId: string; draftOrder: number }; + +// Inline snake-draft pick calculation — mirrors calculatePickInfo in draft-utils.ts +// without creating a circular import (draft-utils imports this module). +function pickInRoundFor(pickNumber: number, teamCount: number): number { + const round = Math.ceil(pickNumber / teamCount); + const rawPickInRound = ((pickNumber - 1) % teamCount) + 1; + const isOddRound = round % 2 === 1; + const teamIndex = isOddRound ? rawPickInRound - 1 : teamCount - rawPickInRound; + return teamIndex + 1; +} + export async function notifyPickMadeOnDiscord(params: { seasonId: string; + leagueId: string; pickedTeamName: string; participantName: string; sportName: string; @@ -14,11 +27,13 @@ export async function notifyPickMadeOnDiscord(params: { round: number; pickInRound: number; isDraftComplete: boolean; - nextDraftSlotTeamId: string | null; + totalTeams: number; + draftSlots: DraftSlot[]; db: ReturnType; }): Promise { const { seasonId, + leagueId, pickedTeamName, participantName, sportName, @@ -26,59 +41,62 @@ export async function notifyPickMadeOnDiscord(params: { round, pickInRound, isDraftComplete, - nextDraftSlotTeamId, + totalTeams, + draftSlots, db, } = params; - try { - const season = await db.query.seasons.findFirst({ + const league = await db.query.leagues.findFirst({ + where: eq(schema.leagues.id, leagueId), + }); + if (!league?.discordWebhookUrl || !league.discordPicksAnnouncementEnabled) return; + + const appUrl = process.env.APP_URL ?? "https://brackt.com"; + const draftUrl = `${appUrl}/leagues/${leagueId}/draft/${seasonId}`; + + let nextTeamName: string | undefined; + let nextOwnerDiscordId: string | undefined; + + if (!isDraftComplete) { + // Read currentPickNumber fresh from DB so we see the post-autodraft-chain state, + // matching the same guarantee sendOnTheClockEmail provides. + const freshSeason = await db.query.seasons.findFirst({ where: eq(schema.seasons.id, seasonId), }); - if (!season) return; - - const league = await db.query.leagues.findFirst({ - where: eq(schema.leagues.id, season.leagueId), - }); - if (!league?.discordWebhookUrl || !league.discordPicksAnnouncementEnabled) return; - - const appUrl = process.env.APP_URL ?? "https://brackt.com"; - const draftUrl = `${appUrl}/leagues/${season.leagueId}/draft/${seasonId}`; - - let nextTeamName: string | undefined; - let nextOwnerDiscordId: string | undefined; - - if (!isDraftComplete && nextDraftSlotTeamId) { - const nextTeam = await db.query.teams.findFirst({ - where: eq(schema.teams.id, nextDraftSlotTeamId), - }); - if (nextTeam) { - nextTeamName = nextTeam.name; - if (nextTeam.ownerId) { - const owner = await db.query.users.findFirst({ - where: eq(schema.users.id, nextTeam.ownerId), - }); - if (owner?.discordPingEnabled) { - const discordIds = await findDiscordIdsByUserIds([owner.id]); - nextOwnerDiscordId = discordIds.get(owner.id); + if (freshSeason) { + const nextPickInRound = pickInRoundFor(freshSeason.currentPickNumber ?? 1, totalTeams); + const nextSlot = draftSlots.find((s) => s.draftOrder === nextPickInRound); + if (nextSlot) { + const nextTeam = await db.query.teams.findFirst({ + where: eq(schema.teams.id, nextSlot.teamId), + }); + if (nextTeam) { + nextTeamName = nextTeam.name; + if (nextTeam.ownerId) { + const owner = await db.query.users.findFirst({ + where: eq(schema.users.id, nextTeam.ownerId), + }); + if (owner?.discordPingEnabled) { + const discordIds = await findDiscordIdsByUserIds([owner.id]); + nextOwnerDiscordId = discordIds.get(owner.id); + } } } } } - - await sendPickAnnouncementNotification({ - webhookUrl: league.discordWebhookUrl, - draftUrl, - pickNumber, - round, - pickInRound, - pickedTeamName, - participantName, - sportName, - nextTeamName, - nextOwnerDiscordId, - isDraftComplete, - }); - } catch (err) { - logger.error("[DiscordPick] Failed to send pick announcement:", err); } + + await sendPickAnnouncementNotification({ + webhookUrl: league.discordWebhookUrl, + draftUrl, + pickNumber, + round, + pickInRound, + pickedTeamName, + participantName, + sportName, + nextTeamName, + nextOwnerDiscordId, + isDraftComplete, + }); }