From d22f22a7c3de2420be3f880bc7a0bcd489e73888 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 20 Feb 2026 21:50:54 +0000 Subject: [PATCH] Fix second round of notification code review issues - Add My Turn Only / All Picks mode granularity so users can choose to only be notified when it's their pick, or for every pick - Remove double top-border by stripping leftover border-t/pt-4/mt-4 wrapper from NotificationSettings (now provided by DraftSidebar) - Remove unused isSupported from hook return value - Add n.onclick = () => window.focus() so clicking a notification brings the draft tab back into focus - Scope localStorage keys to userId to avoid shared-browser conflicts (keys now: draftNotifications-{userId}-{seasonId}) - Add notificationsModeRef alongside sendNotificationRef so mode changes don't trigger full socket handler re-registration https://claude.ai/code/session_0149MvVUYDY6pFUAV1fL4K69 --- app/components/NotificationSettings.tsx | 29 +++++++++- app/hooks/useDraftNotifications.ts | 57 +++++++++++++------ .../leagues/$leagueId.draft.$seasonId.tsx | 20 +++++-- 3 files changed, 83 insertions(+), 23 deletions(-) diff --git a/app/components/NotificationSettings.tsx b/app/components/NotificationSettings.tsx index f1e6cfa..0d48152 100644 --- a/app/components/NotificationSettings.tsx +++ b/app/components/NotificationSettings.tsx @@ -1,15 +1,21 @@ import { Switch } from "~/components/ui/switch"; import { Label } from "~/components/ui/label"; +import { RadioGroup, RadioGroupItem } from "~/components/ui/radio-group"; +import type { NotificationMode } from "~/hooks/useDraftNotifications"; interface NotificationSettingsProps { enabled: boolean; onEnabledChange: (enabled: boolean) => void; + mode: NotificationMode; + onModeChange: (mode: NotificationMode) => void; permissionState: NotificationPermission | "unsupported"; } export function NotificationSettings({ enabled, onEnabledChange, + mode, + onModeChange, permissionState, }: NotificationSettingsProps) { if (permissionState === "unsupported") { @@ -17,7 +23,7 @@ export function NotificationSettings({ } return ( -
+
); } diff --git a/app/hooks/useDraftNotifications.ts b/app/hooks/useDraftNotifications.ts index f62c7f1..cdac59c 100644 --- a/app/hooks/useDraftNotifications.ts +++ b/app/hooks/useDraftNotifications.ts @@ -1,33 +1,43 @@ import { useState, useEffect, useCallback } from "react"; -function getStorageKey(seasonId: string) { - return `draftNotifications-${seasonId}`; +export type NotificationMode = "my_turn" | "all_picks"; + +function getEnabledKey(userId: string, seasonId: string) { + return `draftNotifications-${userId}-${seasonId}`; } -export function useDraftNotifications(seasonId: string) { - const [isSupported, setIsSupported] = useState(false); +function getModeKey(userId: string, seasonId: string) { + return `draftNotificationMode-${userId}-${seasonId}`; +} + +export function useDraftNotifications(seasonId: string, userId: string) { const [permissionState, setPermissionState] = useState< NotificationPermission | "unsupported" >("unsupported"); const [enabled, setEnabledState] = useState(false); + const [mode, setModeState] = useState("my_turn"); - // Check browser support, restore persisted preference, and watch for permission changes + // Check browser support, restore persisted preferences, and watch for permission changes useEffect(() => { if (typeof window === "undefined" || !("Notification" in window)) { return; } - setIsSupported(true); setPermissionState(Notification.permission); - // Restore preference from localStorage only if permission is granted + // Restore preferences from localStorage only if permission is granted if (Notification.permission === "granted") { - const stored = localStorage.getItem(getStorageKey(seasonId)); - if (stored === "true") { + const storedEnabled = localStorage.getItem(getEnabledKey(userId, seasonId)); + if (storedEnabled === "true") { setEnabledState(true); } } + const storedMode = localStorage.getItem(getModeKey(userId, seasonId)); + if (storedMode === "my_turn" || storedMode === "all_picks") { + setModeState(storedMode); + } + // Watch for the user revoking/granting permission in browser settings let permissionStatus: PermissionStatus | null = null; navigator.permissions @@ -51,11 +61,11 @@ export function useDraftNotifications(seasonId: string) { permissionStatus.onchange = null; } }; - }, [seasonId]); + }, [seasonId, userId]); const setEnabled = useCallback( async (value: boolean) => { - if (!isSupported) return; + if (typeof window === "undefined" || !("Notification" in window)) return; if (value) { // Request permission if not yet granted @@ -70,20 +80,29 @@ export function useDraftNotifications(seasonId: string) { } setEnabledState(true); - localStorage.setItem(getStorageKey(seasonId), "true"); + localStorage.setItem(getEnabledKey(userId, seasonId), "true"); } else { setEnabledState(false); - localStorage.setItem(getStorageKey(seasonId), "false"); + localStorage.setItem(getEnabledKey(userId, seasonId), "false"); } }, - [isSupported, seasonId] + [userId, seasonId] + ); + + const setMode = useCallback( + (value: NotificationMode) => { + setModeState(value); + localStorage.setItem(getModeKey(userId, seasonId), value); + }, + [userId, seasonId] ); const sendNotification = useCallback( (title: string, body: string) => { if ( !enabled || - !isSupported || + typeof window === "undefined" || + !("Notification" in window) || Notification.permission !== "granted" || typeof document === "undefined" || !document.hidden @@ -91,19 +110,21 @@ export function useDraftNotifications(seasonId: string) { return; } - new Notification(title, { + const n = new Notification(title, { body, tag: `draft-${seasonId}`, }); + n.onclick = () => window.focus(); }, - [enabled, isSupported, seasonId] + [enabled, seasonId] ); return { - isSupported, permissionState, enabled, setEnabled, + mode, + setMode, sendNotification, }; } diff --git a/app/routes/leagues/$leagueId.draft.$seasonId.tsx b/app/routes/leagues/$leagueId.draft.$seasonId.tsx index 834a5b3..11ee2df 100644 --- a/app/routes/leagues/$leagueId.draft.$seasonId.tsx +++ b/app/routes/leagues/$leagueId.draft.$seasonId.tsx @@ -199,21 +199,28 @@ export default function DraftRoom() { autodraftSettings, userAutodraftSettings, isCommissioner, + currentUserId, } = useLoaderData(); const { isConnected, connectionError, isReconnecting, on, off } = useDraftSocket(season.id, userTeam?.id); const { permissionState: notificationsPermission, enabled: notificationsEnabled, setEnabled: setNotificationsEnabled, + mode: notificationsMode, + setMode: setNotificationsMode, sendNotification, - } = useDraftNotifications(season.id); + } = useDraftNotifications(season.id, currentUserId); - // Use a ref so the socket effect doesn't re-register all handlers just because - // the user toggled notifications (which changes the sendNotification reference). + // Use refs so the socket effect doesn't re-register all handlers when the user + // changes notification settings (which would change the sendNotification reference). const sendNotificationRef = useRef(sendNotification); useEffect(() => { sendNotificationRef.current = sendNotification; }, [sendNotification]); + const notificationsModeRef = useRef(notificationsMode); + useEffect(() => { + notificationsModeRef.current = notificationsMode; + }, [notificationsMode]); const [picks, setPicks] = useState(draftPicks); const [currentPick, setCurrentPick] = useState(season.currentPickNumber || 1); const [searchQuery, setSearchQuery] = useState(""); @@ -388,7 +395,10 @@ export default function DraftRoom() { if (isNowMyTurn) { sendNotificationRef.current(notifTitle, `It's your turn to pick!`); - } else if (data.pick?.team?.id !== userTeam?.id) { + } else if ( + notificationsModeRef.current === "all_picks" && + 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"; @@ -914,6 +924,8 @@ export default function DraftRoom() { }