From 09b9b9bdad1dce7c8417021d31485d7428bad5d1 Mon Sep 17 00:00:00 2001 From: Chris Parsons <438676+chrisparsons83@users.noreply.github.com> Date: Wed, 18 Mar 2026 16:46:07 -0700 Subject: [PATCH] Optimize user data fetching with batch queries and centralize display name logic (#176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fall back to displayName when username is null for Discord webhook Users who sign up via OAuth (Google, GitHub, etc.) without setting a Clerk username have a null `username` field but always have a `displayName` (computed from firstName+lastName or email). Previously, `usernameByClerkId` was filtered to only include users with a non-null username, causing those owners to appear without any identifier in Discord standings messages (e.g. "Liverpool def. Galatasaray" instead of "Liverpool def. Galatasaray (Madmike)"). https://claude.ai/code/session_01VAkeDDVZMYS1DweQnUrRnH * Extract getUserDisplayName helper and use consistently throughout Add a single getUserDisplayName(user) function to app/models/user.ts that encapsulates the username → displayName fallback logic. Replace 9 scattered inline expressions across the codebase (owner-map, scoring-calculator, league routes, settings, invite flow, draft API, Clerk webhook) with calls to the shared helper. No behaviour change — all existing logic preserved, just centralised. https://claude.ai/code/session_01VAkeDDVZMYS1DweQnUrRnH * Fix N+1 user queries in league loader and settings loader Add findUsersByClerkIds() batch function to the user model and replace two separate Promise.all+findUserByClerkId loops (one for owners, one for commissioners) with a single inArray query in both $leagueId.server.ts and $leagueId.settings.tsx. The merged query covers both owner and commissioner IDs in one round-trip. https://claude.ai/code/session_01VAkeDDVZMYS1DweQnUrRnH * Fix N+1 user queries in buildOwnerMap Replace the Promise.all+findUserByClerkId loop with a single findUsersByClerkIds batch query, consistent with the league loader and settings loader fixes. https://claude.ai/code/session_01VAkeDDVZMYS1DweQnUrRnH --------- Co-authored-by: Claude --- app/lib/owner-map.ts | 30 ++++-------- app/models/scoring-calculator.ts | 5 +- app/models/user.ts | 22 ++++++++- app/routes/api/seasons.$seasonId.draft.ts | 3 +- app/routes/api/webhooks/clerk.ts | 6 +-- app/routes/i.$inviteCode.tsx | 3 +- app/routes/leagues/$leagueId.server.ts | 47 +++++++------------ app/routes/leagues/$leagueId.settings.tsx | 46 +++++++++--------- ...d.sports-seasons.$sportsSeasonId.server.ts | 3 +- 9 files changed, 82 insertions(+), 83 deletions(-) diff --git a/app/lib/owner-map.ts b/app/lib/owner-map.ts index 70e118e..e7d2656 100644 --- a/app/lib/owner-map.ts +++ b/app/lib/owner-map.ts @@ -1,4 +1,4 @@ -import { findUserByClerkId } from "~/models/user"; +import { findUsersByClerkIds, getUserDisplayName } from "~/models/user"; /** * Builds a map of teamId -> manager username (or displayName fallback). @@ -7,30 +7,20 @@ import { findUserByClerkId } from "~/models/user"; export async function buildOwnerMap( slots: Array<{ team: { id: string; ownerId: string | null } }> ): Promise> { - const ownerIds = slots - .map((s) => s.team.ownerId) - .filter((id): id is string => id !== null); - const uniqueOwnerIds = [...new Set(ownerIds)]; + const uniqueOwnerIds = [...new Set( + slots.map((s) => s.team.ownerId).filter((id): id is string => id !== null) + )]; - const ownerEntries = await Promise.all( - uniqueOwnerIds.map(async (ownerId) => { - const user = await findUserByClerkId(ownerId); - const name = user?.username || user?.displayName || null; - return name ? { ownerId, name } : null; - }) - ); - - const ownerByClerkId = new Map( - ownerEntries - .filter((e): e is NonNullable => e !== null) - .map((e) => [e.ownerId, e.name]) + const userRows = await findUsersByClerkIds(uniqueOwnerIds); + const userByClerkId = new Map( + userRows + .map((u) => [u.clerkId, getUserDisplayName(u)] as const) + .filter((entry): entry is [string, string] => entry[1] != null) ); const ownerMap: Record = {}; for (const slot of slots) { - const name = slot.team.ownerId - ? ownerByClerkId.get(slot.team.ownerId) - : undefined; + const name = slot.team.ownerId ? userByClerkId.get(slot.team.ownerId) : undefined; if (name) { ownerMap[slot.team.id] = name; } diff --git a/app/models/scoring-calculator.ts b/app/models/scoring-calculator.ts index f09e1af..7b38560 100644 --- a/app/models/scoring-calculator.ts +++ b/app/models/scoring-calculator.ts @@ -6,6 +6,7 @@ import { getSeasonResults } from "./participant-season-result"; import { updateProbabilitiesAfterResult } from "~/services/probability-updater"; import { sendStandingsUpdateNotification, type ScoredMatch } from "~/services/discord"; import { BRACKET_TEMPLATES } from "~/lib/bracket-templates"; +import { getUserDisplayName } from "~/models/user"; /** * Core scoring calculation engine @@ -1313,8 +1314,8 @@ export async function recalculateAffectedLeagues( : []; const usernameByClerkId = new Map( teamOwnerUsers - .filter((u) => u.username != null) - .map((u) => [u.clerkId, u.username!]) + .map((u) => [u.clerkId, getUserDisplayName(u)]) + .filter((entry): entry is [string, string] => entry[1] != null) ); // Build teamId → ownerId map from standings data diff --git a/app/models/user.ts b/app/models/user.ts index fea35d2..0cc3c45 100644 --- a/app/models/user.ts +++ b/app/models/user.ts @@ -1,10 +1,21 @@ -import { eq } from "drizzle-orm"; +import { eq, inArray } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; export type User = typeof schema.users.$inferSelect; export type NewUser = typeof schema.users.$inferInsert; +/** + * Returns the best available display identifier for a user. + * Prefers Clerk username; falls back to displayName (always set from + * firstName+lastName or email prefix). Returns null only if both are absent. + */ +export function getUserDisplayName( + user: Pick +): string | null { + return user.username ?? user.displayName ?? null; +} + export async function createUser(data: NewUser): Promise { const db = database(); const [user] = await db.insert(schema.users).values(data).returning(); @@ -27,6 +38,15 @@ export async function findUserByClerkId( }); } +export async function findUsersByClerkIds(clerkIds: string[]): Promise { + if (clerkIds.length === 0) return []; + const db = database(); + return await db + .select() + .from(schema.users) + .where(inArray(schema.users.clerkId, clerkIds)); +} + export async function updateUser( id: string, data: Partial diff --git a/app/routes/api/seasons.$seasonId.draft.ts b/app/routes/api/seasons.$seasonId.draft.ts index fd80c9c..37316c4 100644 --- a/app/routes/api/seasons.$seasonId.draft.ts +++ b/app/routes/api/seasons.$seasonId.draft.ts @@ -2,6 +2,7 @@ import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { eq, inArray } from "drizzle-orm"; import { calculatePickInfo } from "~/models/draft-utils"; +import { getUserDisplayName } from "~/models/user"; import type { LoaderFunctionArgs } from "react-router"; @@ -77,7 +78,7 @@ export async function loader({ params }: LoaderFunctionArgs) { .from(schema.users) .where(inArray(schema.users.clerkId, allOwnerIds)); for (const owner of owners) { - usernameByClerkId.set(owner.clerkId, owner.username ?? owner.displayName ?? null); + usernameByClerkId.set(owner.clerkId, getUserDisplayName(owner)); } } diff --git a/app/routes/api/webhooks/clerk.ts b/app/routes/api/webhooks/clerk.ts index c1cd1de..448ac85 100644 --- a/app/routes/api/webhooks/clerk.ts +++ b/app/routes/api/webhooks/clerk.ts @@ -1,6 +1,6 @@ import { Webhook } from "svix"; import type { Route } from "./+types/clerk"; -import { findOrCreateUser } from "~/models/user"; +import { findOrCreateUser, getUserDisplayName } from "~/models/user"; export async function action({ request }: Route.ActionArgs) { // Get the webhook secret from environment @@ -62,7 +62,7 @@ export async function action({ request }: Route.ActionArgs) { imageUrl: image_url, }); console.log( - `User created in database: ${user.id} (${user.username || user.displayName})` + `User created in database: ${user.id} (${getUserDisplayName(user)})` ); } else { // Update existing user (or create if doesn't exist) @@ -78,7 +78,7 @@ export async function action({ request }: Route.ActionArgs) { imageUrl: image_url, }); console.log( - `User updated in database: ${user.id} (${user.username || user.displayName})` + `User updated in database: ${user.id} (${getUserDisplayName(user)})` ); } } catch (error) { diff --git a/app/routes/i.$inviteCode.tsx b/app/routes/i.$inviteCode.tsx index 10ae4d9..308c4e7 100644 --- a/app/routes/i.$inviteCode.tsx +++ b/app/routes/i.$inviteCode.tsx @@ -11,6 +11,7 @@ import { claimTeam, isUserLeagueMember, findUserByClerkId, + getUserDisplayName, } from "~/models"; import { Button } from "~/components/ui/button"; import { @@ -103,7 +104,7 @@ export async function action(args: Route.ActionArgs) { // Claim the first available team, setting owner and name atomically const team = availableTeams[0]; - const teamName = `Team ${user.username || user.displayName || "Member"}`; + const teamName = `Team ${getUserDisplayName(user) ?? "Member"}`; await claimTeam(team.id, userId, teamName); // Redirect to league page diff --git a/app/routes/leagues/$leagueId.server.ts b/app/routes/leagues/$leagueId.server.ts index a8d73d8..a28e1b2 100644 --- a/app/routes/leagues/$leagueId.server.ts +++ b/app/routes/leagues/$leagueId.server.ts @@ -7,7 +7,8 @@ import { findCommissionersByLeagueId, isUserLeagueMember, isCommissioner, - findUserByClerkId, + findUsersByClerkIds, + getUserDisplayName, findDraftSlotsBySeasonId, } from "~/models"; import { findCurrentSeasonWithSports } from "~/models/season"; @@ -74,39 +75,25 @@ export async function loader(args: Route.LoaderArgs) { ? await getSeasonStandings(season.id) : []; - // Fetch user data for team owners - const ownerIds = teams - .map((t) => t.ownerId) - .filter((id): id is string => id !== null); - const uniqueOwnerIds = [...new Set(ownerIds)]; - const owners = await Promise.all( - uniqueOwnerIds.map(async (ownerId) => { - const user = await findUserByClerkId(ownerId); - return user - ? { clerkId: ownerId, name: user.username || user.displayName } - : null; - }) - ); + // Batch-fetch all users needed for owner and commissioner maps in one query + const ownerIds = [...new Set(teams.map((t) => t.ownerId).filter((id): id is string => id !== null))]; + const commissionerIds = commissioners.map((c) => c.userId); + const allUserIds = [...new Set([...ownerIds, ...commissionerIds])]; + const userRows = await findUsersByClerkIds(allUserIds); + const userByClerkId = new Map(userRows.map((u) => [u.clerkId, u])); + const ownerMap = new Map( - owners - .filter((o): o is NonNullable => o !== null) - .map((o) => [o.clerkId, o.name]) + ownerIds + .map((id) => [id, userByClerkId.get(id)] as const) + .filter((entry): entry is [string, NonNullable] => entry[1] != null) + .map(([id, user]) => [id, getUserDisplayName(user)]) ); - // Fetch user data for commissioners - const commissionerIds = commissioners.map((c) => c.userId); - const commissionerUsers = await Promise.all( - commissionerIds.map(async (commissionerId) => { - const user = await findUserByClerkId(commissionerId); - return user - ? { clerkId: commissionerId, name: user.username || user.displayName } - : null; - }) - ); const commissionerMap = new Map( - commissionerUsers - .filter((c): c is NonNullable => c !== null) - .map((c) => [c.clerkId, c.name]) + commissionerIds + .map((id) => [id, userByClerkId.get(id)] as const) + .filter((entry): entry is [string, NonNullable] => entry[1] != null) + .map(([id, user]) => [id, getUserDisplayName(user)]) ); // Count available teams diff --git a/app/routes/leagues/$leagueId.settings.tsx b/app/routes/leagues/$leagueId.settings.tsx index c420029..d7fccd3 100644 --- a/app/routes/leagues/$leagueId.settings.tsx +++ b/app/routes/leagues/$leagueId.settings.tsx @@ -15,7 +15,7 @@ import { } from "~/models/commissioner"; import { findCurrentSeasonWithSports, updateSeason } from "~/models/season"; import { findTeamsBySeasonId, deleteTeam, removeTeamOwner, claimTeam } from "~/models/team"; -import { findAllUsers, findUserByClerkId, isUserAdminByClerkId } from "~/models/user"; +import { findAllUsers, findUserByClerkId, findUsersByClerkIds, getUserDisplayName, isUserAdminByClerkId } from "~/models/user"; import { unlinkSportFromSeason, linkMultipleSportsToSeason } from "~/models/season-sport"; import { findAllSportsSeasons } from "~/models/sports-season"; import { findDraftSlotsBySeasonId, setDraftOrder, randomizeDraftOrder } from "~/models/draft-slot"; @@ -112,30 +112,28 @@ export async function loader(args: Route.LoaderArgs) { // Get commissioners for this league with user info const commissioners = await findCommissionersByLeagueId(leagueId); - const commissionerUserData = await Promise.all( - commissioners.map(async (c) => { - const user = await findUserByClerkId(c.userId); - return { - ...c, - userName: user?.username || user?.displayName || "Unknown User", - }; - }) - ); - // Get owner details for teams - const ownerIds = teams - .map((t) => t.ownerId) - .filter((id): id is string => id !== null); - const uniqueOwnerIds = [...new Set(ownerIds)]; - const owners = await Promise.all( - uniqueOwnerIds.map(async (ownerId) => { - const user = await findUserByClerkId(ownerId); - return user - ? { clerkId: ownerId, name: user.username || user.displayName, id: user.id } - : null; + // Batch-fetch all users needed for commissioner and owner lookups in one query + const uniqueOwnerIds = [...new Set(teams.map((t) => t.ownerId).filter((id): id is string => id !== null))]; + const commissionerUserIds = commissioners.map((c) => c.userId); + const allUserIds = [...new Set([...commissionerUserIds, ...uniqueOwnerIds])]; + const userRows = await findUsersByClerkIds(allUserIds); + const userByClerkId = new Map(userRows.map((u) => [u.clerkId, u])); + + const commissionerUserData = commissioners.map((c) => { + const user = userByClerkId.get(c.userId); + return { + ...c, + userName: user ? (getUserDisplayName(user) ?? "Unknown User") : "Unknown User", + }; + }); + + const validOwners = uniqueOwnerIds + .map((ownerId) => { + const user = userByClerkId.get(ownerId); + return user ? { clerkId: ownerId, name: getUserDisplayName(user), id: user.id } : null; }) - ); - const validOwners = owners.filter((o): o is NonNullable => o !== null); + .filter((o): o is NonNullable => o !== null); const ownerMap = new Map(validOwners.map((o) => [o.clerkId, o.name])); // League members (team owners) - available to all commissioners for adding co-commissioners @@ -382,7 +380,7 @@ export async function action(args: Route.ActionArgs) { } try { - const teamName = `Team ${assignedUser.username || assignedUser.displayName || "Member"}`; + const teamName = `Team ${getUserDisplayName(assignedUser) ?? "Member"}`; await claimTeam(teamId, userClerkId, teamName); return { success: true, message: "Owner assigned successfully" }; diff --git a/app/routes/leagues/$leagueId.sports-seasons.$sportsSeasonId.server.ts b/app/routes/leagues/$leagueId.sports-seasons.$sportsSeasonId.server.ts index d647efa..9f9b020 100644 --- a/app/routes/leagues/$leagueId.sports-seasons.$sportsSeasonId.server.ts +++ b/app/routes/leagues/$leagueId.sports-seasons.$sportsSeasonId.server.ts @@ -5,6 +5,7 @@ import { isUserLeagueMember, isCommissioner, findTeamsBySeasonId, + getUserDisplayName, } from "~/models"; import { getDraftPicks } from "~/models/draft-pick"; import { getSeasonResults } from "~/models/participant-season-result"; @@ -98,7 +99,7 @@ export async function loader(args: Route.LoaderArgs) { }) : []; const ownerMap = new Map( - ownerUserRows.map((u) => [u.clerkId, u.username || u.displayName || "Unknown"]) + ownerUserRows.map((u) => [u.clerkId, getUserDisplayName(u) ?? "Unknown"]) ); // Map draft picks to ownership