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

@@ -44,28 +44,26 @@ export function NotificationSettings({
             disabled={permissionState === "denied"}
           />
         </PopoverAnchor>
-        {enabled && (
+        <PopoverContent align="end" className="w-48 p-3">
-          <PopoverContent align="end" className="w-48 p-3">
+          <RadioGroup
-            <RadioGroup
+            value={mode}
-              value={mode}
+            onValueChange={(value) => onModeChange(value as NotificationMode)}
-              onValueChange={(value) => onModeChange(value as NotificationMode)}
+            className="space-y-2"
-              className="space-y-2"
+          >
-            >
+            <div className="flex items-center space-x-2">
-              <div className="flex items-center space-x-2">
+              <RadioGroupItem value="my_turn" id="notif_header_my_turn" />
-                <RadioGroupItem value="my_turn" id="notif_header_my_turn" />
+              <Label htmlFor="notif_header_my_turn" className="text-sm cursor-pointer">
-                <Label htmlFor="notif_header_my_turn" className="text-sm cursor-pointer">
+                My Turn Only
-                  My Turn Only
+              </Label>
-                </Label>
+            </div>
-              </div>
+            <div className="flex items-center space-x-2">
-              <div className="flex items-center space-x-2">
+              <RadioGroupItem value="all_picks" id="notif_header_all_picks" />
-                <RadioGroupItem value="all_picks" id="notif_header_all_picks" />
+              <Label htmlFor="notif_header_all_picks" className="text-sm cursor-pointer">
-                <Label htmlFor="notif_header_all_picks" className="text-sm cursor-pointer">
+                All Picks
-                  All Picks
+              </Label>
-                </Label>
+            </div>
-              </div>
+          </RadioGroup>
-            </RadioGroup>
+        </PopoverContent>
-          </PopoverContent>
-        )}
       </Popover>
     );
   }

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<typeof useFetcher>) {
+  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"}
           />
         </div>
-        {emailFetcher.state === "idle" && emailFetcher.data && (emailFetcher.data as { intent?: string; success?: boolean }).intent === "update-email-draft-notifications" && (emailFetcher.data as { success?: boolean }).success && (
+        {emailSuccess && (
           <p className="text-sm text-green-600">Email notification preference saved.</p>
         )}
       </div>
@@ -106,11 +115,11 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, dra
                 disabled={discordFetcher.state !== "idle"}
               />
             </div>
-            {success && (
+            {discordSuccess && (
               <p className="text-sm text-green-600">Notification preference saved.</p>
             )}
-            {error && (
+            {discordError && (
-              <p className="text-sm text-destructive">{error}</p>
+              <p className="text-sm text-destructive">{discordError}</p>
             )}
           </>
         )}

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
+      // Fire-and-forget: sendOnTheClockEmail fetches the current pick number from
-      const postChainSeason = await db.query.seasons.findFirst({ where: eq(schema.seasons.id, seasonId) });
+      // the DB itself, so it always sees the post-chain state without blocking here.
-      const finalPickNumber = postChainSeason?.currentPickNumber ?? nextPickNumber;
+      sendOnTheClockEmail({ seasonId, totalTeams, draftSlots, db })
-      sendOnTheClockEmail({ seasonId, currentPickNumber: finalPickNumber, totalTeams, totalPicks, draftSlots, db })
         .catch((err) => logger.error("[AutoPick] On-the-clock email failed:", err));
     }
 

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
+    // Fire-and-forget: sendOnTheClockEmail fetches the current pick number from
-    const postChainSeason = await db.query.seasons.findFirst({ where: eq(schema.seasons.id, seasonId) });
+    // the DB itself, so it always sees the post-chain state without blocking here.
-    const finalPickNumber = postChainSeason?.currentPickNumber ?? nextPickNumber;
+    sendOnTheClockEmail({ seasonId, totalTeams, draftSlots, db })
-    const totalPicks = totalTeams * season.draftRounds;
-    sendOnTheClockEmail({ seasonId, currentPickNumber: finalPickNumber, totalTeams, totalPicks, draftSlots, db })
       .catch((err) => logger.error("On-the-clock email failed:", err));
   }
 

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")}
             />
           )}

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) => `<html>${content}</html>`),
+  emailParagraph: vi.fn((html: string) => `<p>${html}</p>`),
+  emailButton: vi.fn((href: string, text: string) => `<a href="${href}">${text}</a>`),
+  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);
+  });
+});

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<typeof database>;
 }): Promise<void> {
-  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
+  // Parallel: team, autodraft settings, and league are all independent reads
-  const team = await db.query.teams.findFirst({
+  const [team, autodraft, league] = await Promise.all([
-    where: eq(schema.teams.id, currentSlot.teamId),
+    db.query.teams.findFirst({ where: eq(schema.teams.id, currentSlot.teamId) }),
-  });
+    db.query.autodraftSettings.findFirst({
-  if (!team?.ownerId) return;
+      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)
+  // Detect consecutive picks: same team picks the very next slot too
-  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)
   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;