From 2f5d36ae48a38de2ffa298ab6a5c3dcb7d694107 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 20 May 2026 22:23:30 +0000 Subject: [PATCH] Address code review feedback on email notification feature - sendOnTheClockEmail now fetches season (and currentPickNumber) internally, removing the blocking postChainSeason pre-fetch from both make-pick.ts and executeAutoPick; callers are now true fire-and-forget with no IIFE needed - Parallel Promise.all for team, autodraftSettings, and league queries reduces sequential DB round-trips from 5 to 3 - Remove team.ownerId type cast; assign to local after the null guard - Combine the two calculatePickInfo calls for the same pick into one - Fix popoverOpen/enabled divergence: remove the {enabled && ...} guard on PopoverContent so popoverOpen is the sole visibility control - Unify NotificationsSection feedback pattern: both Discord and email sections now read success/error directly from their respective fetcher data via a shared feedbackFromFetcher helper; removes the success/error props and the verbose cast that was used for email - Add 13 unit tests for sendOnTheClockEmail covering: early-exit paths (no season, past totalPicks, no owner, notifications disabled, autodraft enabled, no league), single vs back-to-back subject selection, draft room link presence, error logging without throwing, and parallel query execution https://claude.ai/code/session_01LMGxgYvtE3CF8Jf3u2pXgA --- app/components/NotificationSettings.tsx | 42 ++- .../user/settings/NotificationsSection.tsx | 23 +- app/models/draft-utils.ts | 7 +- app/routes/api/draft.make-pick.ts | 8 +- app/routes/settings.tsx | 2 - .../__tests__/draft-email.server.test.ts | 247 ++++++++++++++++++ app/services/draft-email.server.ts | 69 ++--- 7 files changed, 326 insertions(+), 72 deletions(-) create mode 100644 app/services/__tests__/draft-email.server.test.ts diff --git a/app/components/NotificationSettings.tsx b/app/components/NotificationSettings.tsx index 8af9203..a7e746c 100644 --- a/app/components/NotificationSettings.tsx +++ b/app/components/NotificationSettings.tsx @@ -44,28 +44,26 @@ export function NotificationSettings({ disabled={permissionState === "denied"} /> - {enabled && ( - - onModeChange(value as NotificationMode)} - className="space-y-2" - > -
- - -
-
- - -
- - - )} + + onModeChange(value as NotificationMode)} + className="space-y-2" + > +
+ + +
+
+ + +
+ + ); } diff --git a/app/components/user/settings/NotificationsSection.tsx b/app/components/user/settings/NotificationsSection.tsx index 07a2743..ff0c7b5 100644 --- a/app/components/user/settings/NotificationsSection.tsx +++ b/app/components/user/settings/NotificationsSection.tsx @@ -2,16 +2,22 @@ import { useFetcher } from "react-router"; import { Switch } from "~/components/ui/switch"; import { Label } from "~/components/ui/label"; +type ActionResult = { success?: boolean; error?: string }; + +function feedbackFromFetcher(fetcher: ReturnType) { + if (fetcher.state !== "idle" || !fetcher.data) return { success: false, error: undefined }; + const data = fetcher.data as ActionResult; + return { success: !!data.success, error: data.error }; +} + type Props = { discordPingEnabled: boolean; hasDiscordLinked: boolean; draftEmailNotificationsEnabled: boolean; - success?: boolean; - error?: string; onNavigateToAccount: () => void; }; -export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, draftEmailNotificationsEnabled, success, error, onNavigateToAccount }: Props) { +export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, draftEmailNotificationsEnabled, onNavigateToAccount }: Props) { const discordFetcher = useFetcher(); const emailFetcher = useFetcher(); @@ -25,6 +31,9 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, dra ? emailFetcher.formData?.get("draftEmailNotificationsEnabled") === "true" : draftEmailNotificationsEnabled; + const { success: discordSuccess, error: discordError } = feedbackFromFetcher(discordFetcher); + const { success: emailSuccess } = feedbackFromFetcher(emailFetcher); + const handleDiscordToggle = (checked: boolean) => { discordFetcher.submit( { intent: "update-discord-ping", discordPingEnabled: String(checked) }, @@ -67,7 +76,7 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, dra disabled={emailFetcher.state !== "idle"} /> - {emailFetcher.state === "idle" && emailFetcher.data && (emailFetcher.data as { intent?: string; success?: boolean }).intent === "update-email-draft-notifications" && (emailFetcher.data as { success?: boolean }).success && ( + {emailSuccess && (

Email notification preference saved.

)} @@ -106,11 +115,11 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, dra disabled={discordFetcher.state !== "idle"} /> - {success && ( + {discordSuccess && (

Notification preference saved.

)} - {error && ( -

{error}

+ {discordError && ( +

{discordError}

)} )} diff --git a/app/models/draft-utils.ts b/app/models/draft-utils.ts index 0a11b49..1f3c076 100644 --- a/app/models/draft-utils.ts +++ b/app/models/draft-utils.ts @@ -835,10 +835,9 @@ export async function executeAutoPick(params: { db, }); - // After autodraft chain settles, notify whoever is actually on the clock - const postChainSeason = await db.query.seasons.findFirst({ where: eq(schema.seasons.id, seasonId) }); - const finalPickNumber = postChainSeason?.currentPickNumber ?? nextPickNumber; - sendOnTheClockEmail({ seasonId, currentPickNumber: finalPickNumber, totalTeams, totalPicks, draftSlots, db }) + // Fire-and-forget: sendOnTheClockEmail fetches the current pick number from + // the DB itself, so it always sees the post-chain state without blocking here. + sendOnTheClockEmail({ seasonId, totalTeams, draftSlots, db }) .catch((err) => logger.error("[AutoPick] On-the-clock email failed:", err)); } diff --git a/app/routes/api/draft.make-pick.ts b/app/routes/api/draft.make-pick.ts index c8c15c1..e8d8087 100644 --- a/app/routes/api/draft.make-pick.ts +++ b/app/routes/api/draft.make-pick.ts @@ -324,11 +324,9 @@ export async function action(args: ActionFunctionArgs) { }); } - // After autodraft chain settles, notify whoever is actually on the clock - const postChainSeason = await db.query.seasons.findFirst({ where: eq(schema.seasons.id, seasonId) }); - const finalPickNumber = postChainSeason?.currentPickNumber ?? nextPickNumber; - const totalPicks = totalTeams * season.draftRounds; - sendOnTheClockEmail({ seasonId, currentPickNumber: finalPickNumber, totalTeams, totalPicks, draftSlots, db }) + // Fire-and-forget: sendOnTheClockEmail fetches the current pick number from + // the DB itself, so it always sees the post-chain state without blocking here. + sendOnTheClockEmail({ seasonId, totalTeams, draftSlots, db }) .catch((err) => logger.error("On-the-clock email failed:", err)); } diff --git a/app/routes/settings.tsx b/app/routes/settings.tsx index a8a1fd3..a7d4455 100644 --- a/app/routes/settings.tsx +++ b/app/routes/settings.tsx @@ -279,8 +279,6 @@ export default function SettingsPage({ loaderData, actionData }: Route.Component discordPingEnabled={user.discordPingEnabled} hasDiscordLinked={linkedAccounts.some((a) => a.providerId === "discord")} draftEmailNotificationsEnabled={user.draftEmailNotificationsEnabled} - success={ad?.intent === "update-discord-ping" && "success" in ad} - error={ad?.intent === "update-discord-ping" && "error" in ad ? ad.error : undefined} onNavigateToAccount={() => handleSectionChange("account")} /> )} diff --git a/app/services/__tests__/draft-email.server.test.ts b/app/services/__tests__/draft-email.server.test.ts new file mode 100644 index 0000000..c4ba0ef --- /dev/null +++ b/app/services/__tests__/draft-email.server.test.ts @@ -0,0 +1,247 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("~/database/context", () => ({ + database: vi.fn(), +})); + +vi.mock("~/lib/email.server", () => ({ + sendEmail: vi.fn().mockResolvedValue({ error: null }), + wrapInEmailTemplate: vi.fn((content: string) => `${content}`), + emailParagraph: vi.fn((html: string) => `

${html}

`), + emailButton: vi.fn((href: string, text: string) => `${text}`), + escapeHtml: vi.fn((s: string) => s), +})); + +import { sendOnTheClockEmail } from "../draft-email.server"; +import { sendEmail } from "~/lib/email.server"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +const SEASON_ID = "season-1"; +const LEAGUE_ID = "league-1"; +const TEAM_ID = "team-1"; +const TEAM_ID_2 = "team-2"; +const OWNER_ID = "owner-1"; + +/** 2-team, 4-round draft: picks 1,2,3,4,5,6,7,8 */ +const DRAFT_SLOTS_2_TEAM = [ + { teamId: TEAM_ID, draftOrder: 1 }, + { teamId: TEAM_ID_2, draftOrder: 2 }, +]; + +function makeSeason(currentPickNumber: number, draftRounds = 4) { + return { + id: SEASON_ID, + leagueId: LEAGUE_ID, + currentPickNumber, + draftRounds, + }; +} + +function makeOwner(emailNotificationsEnabled = true) { + return { + id: OWNER_ID, + email: "owner@example.com", + draftEmailNotificationsEnabled: emailNotificationsEnabled, + }; +} + +function makeMockDb(overrides: { + season?: object | null; + team?: object | null; + autodraft?: object | null; + league?: object | null; + owner?: object | null; +} = {}) { + const season = "season" in overrides ? overrides.season : makeSeason(1); + const team = "team" in overrides ? overrides.team : { id: TEAM_ID, name: "Team One", ownerId: OWNER_ID }; + const autodraft = "autodraft" in overrides ? overrides.autodraft : null; + const league = "league" in overrides ? overrides.league : { id: LEAGUE_ID, name: "Test League" }; + const owner = "owner" in overrides ? overrides.owner : makeOwner(); + + return { + query: { + seasons: { findFirst: vi.fn().mockResolvedValue(season) }, + teams: { findFirst: vi.fn().mockResolvedValue(team) }, + autodraftSettings: { findFirst: vi.fn().mockResolvedValue(autodraft) }, + leagues: { findFirst: vi.fn().mockResolvedValue(league) }, + users: { findFirst: vi.fn().mockResolvedValue(owner) }, + }, + }; +} + +const BASE_PARAMS = { + seasonId: SEASON_ID, + totalTeams: 2, + draftSlots: DRAFT_SLOTS_2_TEAM, +}; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +beforeEach(() => { + vi.clearAllMocks(); + process.env.APP_URL = "https://test.brackt.com"; +}); + +afterEach(() => { + delete process.env.APP_URL; +}); + +describe("sendOnTheClockEmail", () => { + it("sends an email with the correct subject and recipient", async () => { + const db = makeMockDb({ season: makeSeason(1) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).toHaveBeenCalledWith( + expect.objectContaining({ + to: "owner@example.com", + subject: "You're on the clock in Test League", + }) + ); + }); + + it("returns early when season is not found", async () => { + const db = makeMockDb({ season: null }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("returns early when currentPickNumber exceeds totalPicks", async () => { + // 2 teams × 4 rounds = 8 total picks; pick 9 is past the end + const db = makeMockDb({ season: makeSeason(9, 4) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("returns early when the team has no owner", async () => { + const db = makeMockDb({ team: { id: TEAM_ID, name: "Team One", ownerId: null } }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("returns early when owner has email notifications disabled", async () => { + const db = makeMockDb({ owner: makeOwner(false) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("returns early when autodraft is enabled for the team", async () => { + const db = makeMockDb({ autodraft: { isEnabled: true } }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("returns early when league is not found", async () => { + const db = makeMockDb({ league: null }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).not.toHaveBeenCalled(); + }); + + it("sends back-to-back subject when the same team picks consecutively (snake turnaround)", async () => { + // In a 2-team snake draft, pick 2 (team 2 forward) is followed by pick 3 + // (team 2 again on the reversal). Team 2 picks back-to-back. + const db = makeMockDb({ + season: makeSeason(2), + team: { id: TEAM_ID_2, name: "Team Two", ownerId: OWNER_ID }, + }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).toHaveBeenCalledWith( + expect.objectContaining({ + subject: "You have back-to-back picks in Test League", + }) + ); + }); + + it("sends a regular subject when the next pick goes to a different team", async () => { + // Pick 1 is team 1; pick 2 is team 2 — not consecutive for team 1 + const db = makeMockDb({ season: makeSeason(1) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).toHaveBeenCalledWith( + expect.objectContaining({ + subject: "You're on the clock in Test League", + }) + ); + }); + + it("does not treat the final pick as consecutive even when it is the last slot", async () => { + // Pick 8 is the last pick (2 teams × 4 rounds). No pick 9, so not consecutive. + const db = makeMockDb({ season: makeSeason(8, 4) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + expect(sendEmail).toHaveBeenCalledWith( + expect.objectContaining({ + subject: expect.stringContaining("on the clock"), + }) + ); + }); + + it("includes a link to the draft room in the email", async () => { + const db = makeMockDb({ season: makeSeason(1) }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + const call = vi.mocked(sendEmail).mock.calls[0][0]; + expect(call.html).toContain( + `https://test.brackt.com/leagues/${LEAGUE_ID}/draft/${SEASON_ID}` + ); + }); + + it("logs an error but does not throw when sendEmail fails", async () => { + const db = makeMockDb({ season: makeSeason(1) }); + vi.mocked(sendEmail).mockResolvedValueOnce({ error: new Error("network failure") }); + + await expect( + sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }) + ).resolves.toBeUndefined(); + }); + + it("fetches team, autodraft settings, and league in parallel", async () => { + const order: string[] = []; + const db = makeMockDb({ season: makeSeason(1) }); + + db.query.teams.findFirst = vi.fn().mockImplementation(async () => { + order.push("team"); + return { id: TEAM_ID, name: "Team One", ownerId: OWNER_ID }; + }); + db.query.autodraftSettings.findFirst = vi.fn().mockImplementation(async () => { + order.push("autodraft"); + return null; + }); + db.query.leagues.findFirst = vi.fn().mockImplementation(async () => { + order.push("league"); + return { id: LEAGUE_ID, name: "Test League" }; + }); + + await sendOnTheClockEmail({ ...BASE_PARAMS, db: db as never }); + + // All three should be called; order may vary since they run in parallel + expect(order).toContain("team"); + expect(order).toContain("autodraft"); + expect(order).toContain("league"); + expect(db.query.teams.findFirst).toHaveBeenCalledTimes(1); + expect(db.query.autodraftSettings.findFirst).toHaveBeenCalledTimes(1); + expect(db.query.leagues.findFirst).toHaveBeenCalledTimes(1); + }); +}); diff --git a/app/services/draft-email.server.ts b/app/services/draft-email.server.ts index a126e43..59b8e9b 100644 --- a/app/services/draft-email.server.ts +++ b/app/services/draft-email.server.ts @@ -7,43 +7,60 @@ import type { database } from "~/database/context"; type DraftSlot = { teamId: string; draftOrder: number }; +/** + * Sends an "on the clock" email to the team owner whose turn it is. + * Reads the current pick number directly from the DB so callers never need + * to await a pre-fetch — just fire and forget this after the autodraft chain + * has settled. + */ export async function sendOnTheClockEmail(params: { seasonId: string; - currentPickNumber: number; totalTeams: number; - totalPicks: number; draftSlots: DraftSlot[]; db: ReturnType; }): Promise { - const { seasonId, currentPickNumber, totalTeams, totalPicks, draftSlots, db } = params; + const { seasonId, totalTeams, draftSlots, db } = params; + + // Season is fetched first: we need currentPickNumber + draftRounds + leagueId + // all in one round-trip, and the pick number must be read after the autodraft + // chain has committed its updates to the DB. + const season = await db.query.seasons.findFirst({ + where: eq(schema.seasons.id, seasonId), + }); + if (!season) return; + + const currentPickNumber = season.currentPickNumber ?? 1; + const totalPicks = totalTeams * season.draftRounds; if (currentPickNumber > totalPicks) return; - const { pickInRound } = calculatePickInfo(currentPickNumber, totalTeams); + const { pickInRound, round } = calculatePickInfo(currentPickNumber, totalTeams); const currentSlot = draftSlots.find((s) => s.draftOrder === pickInRound); if (!currentSlot) return; - // Fetch team + owner user - const team = await db.query.teams.findFirst({ - where: eq(schema.teams.id, currentSlot.teamId), - }); - if (!team?.ownerId) return; + // Parallel: team, autodraft settings, and league are all independent reads + const [team, autodraft, league] = await Promise.all([ + db.query.teams.findFirst({ where: eq(schema.teams.id, currentSlot.teamId) }), + db.query.autodraftSettings.findFirst({ + where: and( + eq(schema.autodraftSettings.seasonId, seasonId), + eq(schema.autodraftSettings.teamId, currentSlot.teamId) + ), + }), + db.query.leagues.findFirst({ where: eq(schema.leagues.id, season.leagueId) }), + ]); + // Safety net: skip if autodraft is enabled (pick will be made automatically) + if (autodraft?.isEnabled) return; + if (!team?.ownerId || !league) return; + + const ownerId = team.ownerId; const owner = await db.query.users.findFirst({ - where: eq(schema.users.id, team.ownerId as string), + where: eq(schema.users.id, ownerId), }); if (!owner?.email || !owner.draftEmailNotificationsEnabled) return; - // Safety net: skip if this team has autodraft enabled (their pick will be made automatically) - const autodraft = await db.query.autodraftSettings.findFirst({ - where: and( - eq(schema.autodraftSettings.seasonId, seasonId), - eq(schema.autodraftSettings.teamId, currentSlot.teamId) - ), - }); - if (autodraft?.isEnabled) return; - - // Detect consecutive picks (same team picks the very next slot too) + // Detect consecutive picks: same team picks the very next slot too let isConsecutive = false; if (currentPickNumber + 1 <= totalPicks) { const { pickInRound: nextPickInRound } = calculatePickInfo(currentPickNumber + 1, totalTeams); @@ -53,22 +70,10 @@ export async function sendOnTheClockEmail(params: { } } - // Fetch season + league for context - const season = 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) return; - const appUrl = process.env.APP_URL ?? "https://brackt.com"; const draftUrl = `${appUrl}/leagues/${season.leagueId}/draft/${seasonId}`; const leagueName = escapeHtml(league.name); const teamName = escapeHtml(team.name); - const { round } = calculatePickInfo(currentPickNumber, totalTeams); let subject: string; let preheader: string;