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
This commit is contained in:
parent
f897e7ee05
commit
b7aa1db04a
6 changed files with 42 additions and 28 deletions
|
|
@ -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 = {
|
type Props = {
|
||||||
discordPingEnabled: boolean;
|
discordPingEnabled: boolean;
|
||||||
hasDiscordLinked: boolean;
|
hasDiscordLinked: boolean;
|
||||||
success?: 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 fetcher = useFetcher();
|
||||||
const optimisticEnabled =
|
const optimisticEnabled =
|
||||||
fetcher.state !== "idle"
|
fetcher.state !== "idle"
|
||||||
? fetcher.formData?.get("discordPingEnabled") === "true"
|
? fetcher.formData?.get("discordPingEnabled") === "true"
|
||||||
: discordPingEnabled;
|
: discordPingEnabled;
|
||||||
|
|
||||||
const handleToggle = () => {
|
const handleToggle = (checked: boolean) => {
|
||||||
fetcher.submit(
|
fetcher.submit(
|
||||||
{ intent: "update-discord-ping", discordPingEnabled: String(!optimisticEnabled) },
|
{ intent: "update-discord-ping", discordPingEnabled: String(checked) },
|
||||||
{ method: "post" }
|
{ method: "post" }
|
||||||
);
|
);
|
||||||
};
|
};
|
||||||
|
|
@ -35,41 +39,40 @@ export function NotificationsSection({ discordPingEnabled, hasDiscordLinked, suc
|
||||||
{!hasDiscordLinked ? (
|
{!hasDiscordLinked ? (
|
||||||
<p className="text-sm text-muted-foreground">
|
<p className="text-sm text-muted-foreground">
|
||||||
Link your Discord account in{" "}
|
Link your Discord account in{" "}
|
||||||
<Link to="?section=account" className="text-primary underline-offset-4 hover:underline">
|
<button
|
||||||
|
type="button"
|
||||||
|
onClick={onNavigateToAccount}
|
||||||
|
className="text-primary underline-offset-4 hover:underline"
|
||||||
|
>
|
||||||
Account settings
|
Account settings
|
||||||
</Link>{" "}
|
</button>{" "}
|
||||||
to enable Discord pings for league notifications.
|
to enable Discord pings for league notifications.
|
||||||
</p>
|
</p>
|
||||||
) : (
|
) : (
|
||||||
<>
|
<>
|
||||||
<div className="flex items-center justify-between gap-4">
|
<div className="flex items-center justify-between gap-4">
|
||||||
<div className="space-y-1">
|
<div className="space-y-1">
|
||||||
<p className="text-sm">Ping me in standings updates</p>
|
<Label htmlFor="discord-ping-switch" className="text-sm font-normal">
|
||||||
|
Ping me in standings updates
|
||||||
|
</Label>
|
||||||
<p className="text-xs text-muted-foreground">
|
<p className="text-xs text-muted-foreground">
|
||||||
When enabled, you'll be @mentioned in league Discord notifications and your
|
When enabled, you'll be @mentioned in league Discord notifications and your
|
||||||
Discord display name will appear in place of your brackt username.
|
Discord display name will appear in place of your brackt username.
|
||||||
</p>
|
</p>
|
||||||
</div>
|
</div>
|
||||||
<button
|
<Switch
|
||||||
type="button"
|
id="discord-ping-switch"
|
||||||
role="switch"
|
checked={optimisticEnabled}
|
||||||
aria-checked={optimisticEnabled}
|
onCheckedChange={handleToggle}
|
||||||
onClick={handleToggle}
|
|
||||||
disabled={fetcher.state !== "idle"}
|
disabled={fetcher.state !== "idle"}
|
||||||
className={`relative inline-flex h-6 w-11 shrink-0 cursor-pointer rounded-full border-2 border-transparent transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 ${
|
/>
|
||||||
optimisticEnabled ? "bg-primary" : "bg-input"
|
|
||||||
}`}
|
|
||||||
>
|
|
||||||
<span
|
|
||||||
className={`pointer-events-none block h-5 w-5 rounded-full bg-background shadow-lg ring-0 transition-transform ${
|
|
||||||
optimisticEnabled ? "translate-x-5" : "translate-x-0"
|
|
||||||
}`}
|
|
||||||
/>
|
|
||||||
</button>
|
|
||||||
</div>
|
</div>
|
||||||
{success && (
|
{success && (
|
||||||
<p className="text-sm text-green-600">Notification preference saved.</p>
|
<p className="text-sm text-green-600">Notification preference saved.</p>
|
||||||
)}
|
)}
|
||||||
|
{error && (
|
||||||
|
<p className="text-sm text-destructive">{error}</p>
|
||||||
|
)}
|
||||||
</>
|
</>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|
|
||||||
|
|
@ -1507,14 +1507,13 @@ export async function recalculateAffectedLeagues(
|
||||||
const winnerScoreChanged = winnerTeamId ? changedTeamIds.has(winnerTeamId) : false;
|
const winnerScoreChanged = winnerTeamId ? changedTeamIds.has(winnerTeamId) : false;
|
||||||
const winnerOwnerId = lookupOwner(m.winnerId);
|
const winnerOwnerId = lookupOwner(m.winnerId);
|
||||||
const loserOwnerId = lookupOwner(m.loserId);
|
const loserOwnerId = lookupOwner(m.loserId);
|
||||||
const showWinner = winnerScoreChanged;
|
|
||||||
const showLoser = isLoserNotifiable(m.loserId, loserTeamId, changedTeamIds, finalizedLoserIds);
|
const showLoser = isLoserNotifiable(m.loserId, loserTeamId, changedTeamIds, finalizedLoserIds);
|
||||||
return {
|
return {
|
||||||
winnerName: m.winnerName ?? "",
|
winnerName: m.winnerName ?? "",
|
||||||
loserName: m.loserName ?? "",
|
loserName: m.loserName ?? "",
|
||||||
winnerUsername: showWinner && winnerOwnerId ? usernameByUserId.get(winnerOwnerId) : undefined,
|
winnerUsername: winnerScoreChanged && winnerOwnerId ? usernameByUserId.get(winnerOwnerId) : undefined,
|
||||||
loserUsername: showLoser && loserOwnerId ? usernameByUserId.get(loserOwnerId) : 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,
|
loserDiscordUserId: showLoser && loserOwnerId ? discordIdByUserId.get(loserOwnerId) : undefined,
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -254,6 +254,7 @@ export async function action(args: Route.ActionArgs) {
|
||||||
]),
|
]),
|
||||||
eventName: "Test Notification",
|
eventName: "Test Notification",
|
||||||
scoredMatches: [
|
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" },
|
{ winnerName: "Team Alpha", loserName: "Team Beta", winnerUsername: "manager1", loserUsername: "manager2" },
|
||||||
],
|
],
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -45,6 +45,7 @@ type ActionData =
|
||||||
| { intent: "update-profile"; success: true }
|
| { intent: "update-profile"; success: true }
|
||||||
| { intent: "update-profile"; error: string }
|
| { intent: "update-profile"; error: string }
|
||||||
| { intent: "update-discord-ping"; success: true }
|
| { intent: "update-discord-ping"; success: true }
|
||||||
|
| { intent: "update-discord-ping"; error: string }
|
||||||
| { intent: "request-data"; dataRequestSuccess: true }
|
| { intent: "request-data"; dataRequestSuccess: true }
|
||||||
| { intent: "request-data"; dataRequestError: string }
|
| { intent: "request-data"; dataRequestError: string }
|
||||||
| { intent: string; error: string };
|
| { intent: string; error: string };
|
||||||
|
|
@ -130,6 +131,12 @@ export async function action(args: Route.ActionArgs): Promise<ActionData | Respo
|
||||||
|
|
||||||
if (intent === "update-discord-ping") {
|
if (intent === "update-discord-ping") {
|
||||||
const enabled = formData.get("discordPingEnabled") === "true";
|
const enabled = formData.get("discordPingEnabled") === "true";
|
||||||
|
if (enabled) {
|
||||||
|
const accounts = await findLinkedAccountsByUserId(session.user.id);
|
||||||
|
if (!accounts.some((a) => a.providerId === "discord")) {
|
||||||
|
return { intent: "update-discord-ping", error: "Link your Discord account first." };
|
||||||
|
}
|
||||||
|
}
|
||||||
await updateUser(session.user.id, { discordPingEnabled: enabled });
|
await updateUser(session.user.id, { discordPingEnabled: enabled });
|
||||||
return { intent: "update-discord-ping", success: true };
|
return { intent: "update-discord-ping", success: true };
|
||||||
}
|
}
|
||||||
|
|
@ -245,6 +252,8 @@ export default function SettingsPage({ loaderData, actionData }: Route.Component
|
||||||
discordPingEnabled={user.discordPingEnabled}
|
discordPingEnabled={user.discordPingEnabled}
|
||||||
hasDiscordLinked={linkedAccounts.some((a) => a.providerId === "discord")}
|
hasDiscordLinked={linkedAccounts.some((a) => a.providerId === "discord")}
|
||||||
success={ad?.intent === "update-discord-ping" && "success" in ad}
|
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" && <ApiSection />}
|
{activeSection === "api" && <ApiSection />}
|
||||||
|
|
|
||||||
|
|
@ -180,8 +180,10 @@ export async function sendStandingsUpdateNotification({
|
||||||
};
|
};
|
||||||
|
|
||||||
if (pingIds.length > 0) {
|
if (pingIds.length > 0) {
|
||||||
payload.content = pingIds.map((id) => `<@${id}>`).join(" ");
|
// Discord caps allowed_mentions.users at 100; slice to avoid a rejected payload.
|
||||||
payload.allowed_mentions = { parse: [], users: pingIds };
|
const cappedIds = pingIds.slice(0, 100);
|
||||||
|
payload.content = cappedIds.map((id) => `<@${id}>`).join(" ");
|
||||||
|
payload.allowed_mentions = { parse: [], users: cappedIds };
|
||||||
}
|
}
|
||||||
|
|
||||||
await sendDiscordWebhook(webhookUrl, payload);
|
await sendDiscordWebhook(webhookUrl, payload);
|
||||||
|
|
|
||||||
|
|
@ -7,7 +7,7 @@ export const users = pgTable("users", {
|
||||||
email: varchar("email", { length: 255 }).notNull(),
|
email: varchar("email", { length: 255 }).notNull(),
|
||||||
emailVerified: boolean("email_verified").notNull().default(false),
|
emailVerified: boolean("email_verified").notNull().default(false),
|
||||||
username: varchar("username", { length: 255 }),
|
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
|
imageUrl: varchar("image_url", { length: 512 }), // mapped as BetterAuth's "image" field
|
||||||
flagConfig: jsonb("flag_config").$type<{ pattern: string; colors: string[] }>(),
|
flagConfig: jsonb("flag_config").$type<{ pattern: string; colors: string[] }>(),
|
||||||
customAvatarUrl: varchar("custom_avatar_url", { length: 512 }),
|
customAvatarUrl: varchar("custom_avatar_url", { length: 512 }),
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue