From 30b5fced8cb104269ae91faec792798ca42a048a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 21:45:35 +0000 Subject: [PATCH] Fix all code review issues with push notifications - Extract snake draft order calculation to shared getTeamForPick() helper in lib/draft-order.ts, removing the duplicated logic - Fix notification firing on user's own picks (guard with team id check) - Fix notification firing after draft is complete (early return) - Use a ref for sendNotification to prevent full socket handler re-registration every time the toggle is changed - Add permission drift listener via Permissions API so the UI reacts if the user revokes notification permission in browser settings - Add SSR guard for document.hidden in sendNotification - Remove redundant isSupported prop from NotificationSettings; derive unsupported state from permissionState === "unsupported" - Move NotificationSettings out of QueueSection prop-drilling path; render it via a new settingsSection slot on DraftSidebar instead https://claude.ai/code/session_0149MvVUYDY6pFUAV1fL4K69 --- app/components/DraftSidebar.tsx | 9 +++ app/components/NotificationSettings.tsx | 4 +- app/components/draft/QueueSection.tsx | 17 ----- app/hooks/useDraftNotifications.ts | 27 +++++++- app/lib/draft-order.ts | 18 +++++ .../leagues/$leagueId.draft.$seasonId.tsx | 65 +++++++++---------- 6 files changed, 83 insertions(+), 57 deletions(-) create mode 100644 app/lib/draft-order.ts diff --git a/app/components/DraftSidebar.tsx b/app/components/DraftSidebar.tsx index ead39bd..7e346e8 100644 --- a/app/components/DraftSidebar.tsx +++ b/app/components/DraftSidebar.tsx @@ -14,6 +14,7 @@ interface DraftSidebarProps { onCollapsedChange: (collapsed: boolean) => void; queueSection: ReactNode; recentPicksSection: ReactNode; + settingsSection?: ReactNode; className?: string; } @@ -22,6 +23,7 @@ export function DraftSidebar({ onCollapsedChange, queueSection, recentPicksSection, + settingsSection, className, }: DraftSidebarProps) { if (collapsed) { @@ -94,6 +96,13 @@ export function DraftSidebar({ + {/* Settings section (e.g. notification toggle) */} + {settingsSection && ( +
+ {settingsSection} +
+ )} + {/* Collapse button at bottom */}
); } diff --git a/app/hooks/useDraftNotifications.ts b/app/hooks/useDraftNotifications.ts index 51f9209..f62c7f1 100644 --- a/app/hooks/useDraftNotifications.ts +++ b/app/hooks/useDraftNotifications.ts @@ -11,7 +11,7 @@ export function useDraftNotifications(seasonId: string) { >("unsupported"); const [enabled, setEnabledState] = useState(false); - // Check browser support and restore persisted preference + // Check browser support, restore persisted preference, and watch for permission changes useEffect(() => { if (typeof window === "undefined" || !("Notification" in window)) { return; @@ -27,6 +27,30 @@ export function useDraftNotifications(seasonId: string) { setEnabledState(true); } } + + // Watch for the user revoking/granting permission in browser settings + let permissionStatus: PermissionStatus | null = null; + navigator.permissions + .query({ name: "notifications" }) + .then((status) => { + permissionStatus = status; + status.onchange = () => { + setPermissionState(status.state as NotificationPermission); + // If permission was revoked, disable notifications + if (status.state !== "granted") { + setEnabledState(false); + } + }; + }) + .catch(() => { + // Permissions API not available in all environments; silently ignore + }); + + return () => { + if (permissionStatus) { + permissionStatus.onchange = null; + } + }; }, [seasonId]); const setEnabled = useCallback( @@ -61,6 +85,7 @@ export function useDraftNotifications(seasonId: string) { !enabled || !isSupported || Notification.permission !== "granted" || + typeof document === "undefined" || !document.hidden ) { return; diff --git a/app/lib/draft-order.ts b/app/lib/draft-order.ts new file mode 100644 index 0000000..e474bca --- /dev/null +++ b/app/lib/draft-order.ts @@ -0,0 +1,18 @@ +/** + * Returns the draft slot for a given pick number in a snake draft. + */ +export function getTeamForPick( + pickNumber: number, + draftSlots: T[] +): T | undefined { + const totalTeams = draftSlots.length; + if (totalTeams === 0) return undefined; + + const round = Math.ceil(pickNumber / totalTeams); + const isEvenRound = round % 2 === 0; + let pickInRound = ((pickNumber - 1) % totalTeams) + 1; + if (isEvenRound) { + pickInRound = totalTeams - pickInRound + 1; + } + return draftSlots.find((slot) => slot.draftOrder === pickInRound); +} diff --git a/app/routes/leagues/$leagueId.draft.$seasonId.tsx b/app/routes/leagues/$leagueId.draft.$seasonId.tsx index 729a913..834a5b3 100644 --- a/app/routes/leagues/$leagueId.draft.$seasonId.tsx +++ b/app/routes/leagues/$leagueId.draft.$seasonId.tsx @@ -3,7 +3,7 @@ import { eq, and, asc, desc, inArray } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; import { useDraftSocket } from "~/hooks/useDraftSocket"; -import { useEffect, useState, useMemo } from "react"; +import { useEffect, useState, useMemo, useRef } from "react"; import { Card } from "~/components/ui/card"; import { Badge } from "~/components/ui/badge"; import { Button } from "~/components/ui/button"; @@ -18,7 +18,9 @@ import { SidebarRecentPicks } from "~/components/draft/SidebarRecentPicks"; import { DraftGridSection } from "~/components/draft/DraftGridSection"; import { ConnectionOverlay } from "~/components/draft/ConnectionOverlay"; import { calculateDraftEligibility } from "~/lib/draft-eligibility"; +import { getTeamForPick } from "~/lib/draft-order"; import { useDraftNotifications } from "~/hooks/useDraftNotifications"; +import { NotificationSettings } from "~/components/NotificationSettings"; import { toast } from "sonner"; export async function loader(args: any) { @@ -200,12 +202,18 @@ export default function DraftRoom() { } = useLoaderData(); const { isConnected, connectionError, isReconnecting, on, off } = useDraftSocket(season.id, userTeam?.id); const { - isSupported: notificationsSupported, permissionState: notificationsPermission, enabled: notificationsEnabled, setEnabled: setNotificationsEnabled, sendNotification, } = useDraftNotifications(season.id); + + // Use a ref so the socket effect doesn't re-register all handlers just because + // the user toggled notifications (which changes the sendNotification reference). + const sendNotificationRef = useRef(sendNotification); + useEffect(() => { + sendNotificationRef.current = sendNotification; + }, [sendNotification]); const [picks, setPicks] = useState(draftPicks); const [currentPick, setCurrentPick] = useState(season.currentPickNumber || 1); const [searchQuery, setSearchQuery] = useState(""); @@ -369,31 +377,22 @@ export default function DraftRoom() { setPicks((prev: any) => [...prev, data.pick]); setCurrentPick(data.nextPickNumber); - // Send browser notification - const pickerName = data.pick?.team?.name || "A team"; - const participantName = data.pick?.participant?.name || "a participant"; - const notifTitle = `${season.league.name} Draft`; + // No meaningful "next turn" notification once the draft is over + if (data.isDraftComplete) return; - // Check if the next pick is the current user's turn - const nextPick = data.nextPickNumber; - const totalTeams = draftSlots.length; - const nextRound = Math.ceil(nextPick / totalTeams); - const isEven = nextRound % 2 === 0; - let nextPickInRound = ((nextPick - 1) % totalTeams) + 1; - if (isEven) { - nextPickInRound = totalTeams - nextPickInRound + 1; - } - const nextSlot = draftSlots.find( - (slot) => slot.draftOrder === nextPickInRound - ); + const notifTitle = `${season.league.name} Draft`; + const nextSlot = getTeamForPick(data.nextPickNumber, draftSlots); const isNowMyTurn = !!( userTeam && nextSlot && nextSlot.team.id === userTeam.id ); if (isNowMyTurn) { - sendNotification(notifTitle, `It's your turn to pick!`); - } else { - sendNotification( + sendNotificationRef.current(notifTitle, `It's your turn to pick!`); + } else if (data.pick?.team?.id !== userTeam?.id) { + // Only notify about other teams' picks, not the user's own + const pickerName = data.pick?.team?.name || "A team"; + const participantName = data.pick?.participant?.name || "a participant"; + sendNotificationRef.current( notifTitle, `${pickerName} picked ${participantName}` ); @@ -485,7 +484,7 @@ export default function DraftRoom() { off("connected-teams-list", handleConnectedTeamsList); off("participant-removed-from-queues", handleParticipantRemovedFromQueues); }; - }, [on, off, currentPick, userTeam, sendNotification, draftSlots, season.league.name]); + }, [on, off, currentPick, userTeam, draftSlots, season.league.name]); // Persist sidebar collapsed state useEffect(() => { @@ -695,16 +694,7 @@ export default function DraftRoom() { const currentRound = Math.ceil(currentPick / draftSlots.length); // Determine whose turn it is - const totalTeams = draftSlots.length; - const isSnakeDraft = true; - const isEvenRound = currentRound % 2 === 0; - let pickInRound = ((currentPick - 1) % totalTeams) + 1; - if (isSnakeDraft && isEvenRound) { - pickInRound = totalTeams - pickInRound + 1; - } - const currentDraftSlot = draftSlots.find( - (slot) => slot.draftOrder === pickInRound - ); + const currentDraftSlot = getTeamForPick(currentPick, draftSlots); const isMyTurn = !!( userTeam && currentDraftSlot && currentDraftSlot.team.id === userTeam.id ); @@ -915,15 +905,18 @@ export default function DraftRoom() { setUserAutodraft({ isEnabled, mode }); }} onReorder={handleReorderQueue} - notificationsEnabled={notificationsEnabled} - onNotificationsEnabledChange={setNotificationsEnabled} - notificationsSupported={notificationsSupported} - notificationsPermission={notificationsPermission} /> } recentPicksSection={ } + settingsSection={ + + } /> )}