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
This commit is contained in:
Claude 2026-02-20 21:50:54 +00:00
parent 30b5fced8c
commit d22f22a7c3
No known key found for this signature in database
3 changed files with 83 additions and 23 deletions

View file

@ -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 (
<div className="border-t pt-4 mt-4">
<div>
<div className="flex items-center justify-between">
<Label htmlFor="notifications-switch" className="text-sm font-semibold">
Notifications
@ -36,6 +42,27 @@ export function NotificationSettings({
enable.
</p>
)}
{enabled && (
<RadioGroup
value={mode}
onValueChange={(value) => onModeChange(value as NotificationMode)}
className="space-y-2 mt-3"
>
<div className="flex items-center space-x-2">
<RadioGroupItem value="my_turn" id="notif_my_turn" />
<Label htmlFor="notif_my_turn" className="text-sm cursor-pointer">
My Turn Only
</Label>
</div>
<div className="flex items-center space-x-2">
<RadioGroupItem value="all_picks" id="notif_all_picks" />
<Label htmlFor="notif_all_picks" className="text-sm cursor-pointer">
All Picks
</Label>
</div>
</RadioGroup>
)}
</div>
);
}

View file

@ -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<NotificationMode>("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,
};
}

View file

@ -199,21 +199,28 @@ export default function DraftRoom() {
autodraftSettings,
userAutodraftSettings,
isCommissioner,
currentUserId,
} = useLoaderData<typeof loader>();
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() {
<NotificationSettings
enabled={notificationsEnabled}
onEnabledChange={setNotificationsEnabled}
mode={notificationsMode}
onModeChange={setNotificationsMode}
permissionState={notificationsPermission}
/>
}