From b7aa1db04a06a1c22934384b9e0a6d1f282dca37 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 15 May 2026 17:00:27 +0000 Subject: [PATCH] Address code review feedback on Discord ping feature - Fix broken Account settings link in NotificationsSection: replace the non-functional ?section=account href with an onNavigateToAccount callback that drives the parent's useState-based section switcher - Replace hand-rolled toggle button with the project's Switch component (Radix UI) for consistent sizing, accessibility, and dark-mode support - Guard update-discord-ping action: return an error if the user attempts to enable pings without a Discord account linked - Surface action error in NotificationsSection UI - Cap allowed_mentions.users at 100 to avoid Discord rejecting the payload in large leagues - Remove the showWinner alias in scoring-calculator; inline winnerScoreChanged - Add comment to league settings test notification explaining why discordUserId is omitted - Clarify database schema comment: displayName is write-only from BetterAuth https://claude.ai/code/session_01NUv93WRrufHhpZSMyY4bjs --- .../user/settings/NotificationsSection.tsx | 47 ++++++++++--------- app/models/scoring-calculator.ts | 5 +- .../leagues/$leagueId.settings.server.ts | 1 + app/routes/settings.tsx | 9 ++++ app/services/discord.ts | 6 ++- database/schema.ts | 2 +- 6 files changed, 42 insertions(+), 28 deletions(-) diff --git a/app/components/user/settings/NotificationsSection.tsx b/app/components/user/settings/NotificationsSection.tsx index 7b9cf03..dcc43cf 100644 --- a/app/components/user/settings/NotificationsSection.tsx +++ b/app/components/user/settings/NotificationsSection.tsx @@ -1,21 +1,25 @@ -import { useFetcher, Link } from "react-router"; +import { useFetcher } from "react-router"; +import { Switch } from "~/components/ui/switch"; +import { Label } from "~/components/ui/label"; type Props = { discordPingEnabled: boolean; hasDiscordLinked: boolean; success?: boolean; + error?: string; + onNavigateToAccount: () => void; }; -export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, success }: Props) { +export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, success, error, onNavigateToAccount }: Props) { const fetcher = useFetcher(); const optimisticEnabled = fetcher.state !== "idle" ? fetcher.formData?.get("discordPingEnabled") === "true" : discordPingEnabled; - const handleToggle = () => { + const handleToggle = (checked: boolean) => { fetcher.submit( - { intent: "update-discord-ping", discordPingEnabled: String(!optimisticEnabled) }, + { intent: "update-discord-ping", discordPingEnabled: String(checked) }, { method: "post" } ); }; @@ -35,41 +39,40 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, suc {!hasDiscordLinked ? (

Link your Discord account in{" "} - + {" "} to enable Discord pings for league notifications.

) : ( <>
-

Ping me in standings updates

+

When enabled, you'll be @mentioned in league Discord notifications and your Discord display name will appear in place of your brackt username.

- + />
{success && (

Notification preference saved.

)} + {error && ( +

{error}

+ )} )} diff --git a/app/models/scoring-calculator.ts b/app/models/scoring-calculator.ts index 8a3d140..4259c39 100644 --- a/app/models/scoring-calculator.ts +++ b/app/models/scoring-calculator.ts @@ -1507,14 +1507,13 @@ export async function recalculateAffectedLeagues( const winnerScoreChanged = winnerTeamId ? changedTeamIds.has(winnerTeamId) : false; const winnerOwnerId = lookupOwner(m.winnerId); const loserOwnerId = lookupOwner(m.loserId); - const showWinner = winnerScoreChanged; const showLoser = isLoserNotifiable(m.loserId, loserTeamId, changedTeamIds, finalizedLoserIds); return { winnerName: m.winnerName ?? "", loserName: m.loserName ?? "", - winnerUsername: showWinner && winnerOwnerId ? usernameByUserId.get(winnerOwnerId) : undefined, + winnerUsername: winnerScoreChanged && winnerOwnerId ? usernameByUserId.get(winnerOwnerId) : undefined, loserUsername: showLoser && loserOwnerId ? usernameByUserId.get(loserOwnerId) : undefined, - winnerDiscordUserId: showWinner && winnerOwnerId ? discordIdByUserId.get(winnerOwnerId) : undefined, + winnerDiscordUserId: winnerScoreChanged && winnerOwnerId ? discordIdByUserId.get(winnerOwnerId) : undefined, loserDiscordUserId: showLoser && loserOwnerId ? discordIdByUserId.get(loserOwnerId) : undefined, }; }); diff --git a/app/routes/leagues/$leagueId.settings.server.ts b/app/routes/leagues/$leagueId.settings.server.ts index 9eb0d64..c9d9029 100644 --- a/app/routes/leagues/$leagueId.settings.server.ts +++ b/app/routes/leagues/$leagueId.settings.server.ts @@ -254,6 +254,7 @@ export async function action(args: Route.ActionArgs) { ]), eventName: "Test Notification", scoredMatches: [ + // discordUserId intentionally omitted — we don't know the commissioner's Discord ID at test time { winnerName: "Team Alpha", loserName: "Team Beta", winnerUsername: "manager1", loserUsername: "manager2" }, ], }); diff --git a/app/routes/settings.tsx b/app/routes/settings.tsx index 9fb5b60..75e3a94 100644 --- a/app/routes/settings.tsx +++ b/app/routes/settings.tsx @@ -45,6 +45,7 @@ type ActionData = | { intent: "update-profile"; success: true } | { intent: "update-profile"; error: string } | { intent: "update-discord-ping"; success: true } + | { intent: "update-discord-ping"; error: string } | { intent: "request-data"; dataRequestSuccess: true } | { intent: "request-data"; dataRequestError: string } | { intent: string; error: string }; @@ -130,6 +131,12 @@ export async function action(args: Route.ActionArgs): Promise a.providerId === "discord")) { + return { intent: "update-discord-ping", error: "Link your Discord account first." }; + } + } await updateUser(session.user.id, { discordPingEnabled: enabled }); return { intent: "update-discord-ping", success: true }; } @@ -245,6 +252,8 @@ export default function SettingsPage({ loaderData, actionData }: Route.Component discordPingEnabled={user.discordPingEnabled} hasDiscordLinked={linkedAccounts.some((a) => a.providerId === "discord")} success={ad?.intent === "update-discord-ping" && "success" in ad} + error={ad?.intent === "update-discord-ping" && "error" in ad ? ad.error : undefined} + onNavigateToAccount={() => handleSectionChange("account")} /> )} {activeSection === "api" && } diff --git a/app/services/discord.ts b/app/services/discord.ts index cfa5ceb..70126c5 100644 --- a/app/services/discord.ts +++ b/app/services/discord.ts @@ -180,8 +180,10 @@ export async function sendStandingsUpdateNotification({ }; if (pingIds.length > 0) { - payload.content = pingIds.map((id) => `<@${id}>`).join(" "); - payload.allowed_mentions = { parse: [], users: pingIds }; + // Discord caps allowed_mentions.users at 100; slice to avoid a rejected payload. + const cappedIds = pingIds.slice(0, 100); + payload.content = cappedIds.map((id) => `<@${id}>`).join(" "); + payload.allowed_mentions = { parse: [], users: cappedIds }; } await sendDiscordWebhook(webhookUrl, payload); diff --git a/database/schema.ts b/database/schema.ts index eb2f164..8d0cf8f 100644 --- a/database/schema.ts +++ b/database/schema.ts @@ -7,7 +7,7 @@ export const users = pgTable("users", { email: varchar("email", { length: 255 }).notNull(), emailVerified: boolean("email_verified").notNull().default(false), username: varchar("username", { length: 255 }), - displayName: varchar("display_name", { length: 255 }), // mapped as BetterAuth's "name" field + displayName: varchar("display_name", { length: 255 }), // written by BetterAuth on OAuth login ("name" field); not user-editable imageUrl: varchar("image_url", { length: 512 }), // mapped as BetterAuth's "image" field flagConfig: jsonb("flag_config").$type<{ pattern: string; colors: string[] }>(), customAvatarUrl: varchar("custom_avatar_url", { length: 512 }),