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
This commit is contained in:
Claude 2026-02-20 21:45:35 +00:00
parent c89c1fd973
commit 30b5fced8c
No known key found for this signature in database
6 changed files with 83 additions and 57 deletions

View file

@ -14,6 +14,7 @@ interface DraftSidebarProps {
onCollapsedChange: (collapsed: boolean) => void; onCollapsedChange: (collapsed: boolean) => void;
queueSection: ReactNode; queueSection: ReactNode;
recentPicksSection: ReactNode; recentPicksSection: ReactNode;
settingsSection?: ReactNode;
className?: string; className?: string;
} }
@ -22,6 +23,7 @@ export function DraftSidebar({
onCollapsedChange, onCollapsedChange,
queueSection, queueSection,
recentPicksSection, recentPicksSection,
settingsSection,
className, className,
}: DraftSidebarProps) { }: DraftSidebarProps) {
if (collapsed) { if (collapsed) {
@ -94,6 +96,13 @@ export function DraftSidebar({
</AccordionItem> </AccordionItem>
</Accordion> </Accordion>
{/* Settings section (e.g. notification toggle) */}
{settingsSection && (
<div className="border-t border-border px-4 py-3 flex-shrink-0">
{settingsSection}
</div>
)}
{/* Collapse button at bottom */} {/* Collapse button at bottom */}
<div className="border-t border-border p-2 flex-shrink-0"> <div className="border-t border-border p-2 flex-shrink-0">
<Button <Button

View file

@ -4,17 +4,15 @@ import { Label } from "~/components/ui/label";
interface NotificationSettingsProps { interface NotificationSettingsProps {
enabled: boolean; enabled: boolean;
onEnabledChange: (enabled: boolean) => void; onEnabledChange: (enabled: boolean) => void;
isSupported: boolean;
permissionState: NotificationPermission | "unsupported"; permissionState: NotificationPermission | "unsupported";
} }
export function NotificationSettings({ export function NotificationSettings({
enabled, enabled,
onEnabledChange, onEnabledChange,
isSupported,
permissionState, permissionState,
}: NotificationSettingsProps) { }: NotificationSettingsProps) {
if (!isSupported) { if (permissionState === "unsupported") {
return null; return null;
} }

View file

@ -1,7 +1,6 @@
import { Button } from "~/components/ui/button"; import { Button } from "~/components/ui/button";
import { Badge } from "~/components/ui/badge"; import { Badge } from "~/components/ui/badge";
import { AutodraftSettings } from "~/components/AutodraftSettings"; import { AutodraftSettings } from "~/components/AutodraftSettings";
import { NotificationSettings } from "~/components/NotificationSettings";
import { import {
DndContext, DndContext,
closestCenter, closestCenter,
@ -49,10 +48,6 @@ interface QueueSectionProps {
onRemoveFromQueue: (queueId: string) => void; onRemoveFromQueue: (queueId: string) => void;
onAutodraftUpdate: (isEnabled: boolean, mode: "next_pick" | "while_on") => void; onAutodraftUpdate: (isEnabled: boolean, mode: "next_pick" | "while_on") => void;
onReorder: (participantIds: string[]) => void; onReorder: (participantIds: string[]) => void;
notificationsEnabled: boolean;
onNotificationsEnabledChange: (enabled: boolean) => void;
notificationsSupported: boolean;
notificationsPermission: NotificationPermission | "unsupported";
} }
// Sortable queue item component // Sortable queue item component
@ -135,10 +130,6 @@ export function QueueSection({
onRemoveFromQueue, onRemoveFromQueue,
onAutodraftUpdate, onAutodraftUpdate,
onReorder, onReorder,
notificationsEnabled,
onNotificationsEnabledChange,
notificationsSupported,
notificationsPermission,
}: QueueSectionProps) { }: QueueSectionProps) {
const sensors = useSensors( const sensors = useSensors(
useSensor(PointerSensor), useSensor(PointerSensor),
@ -205,14 +196,6 @@ export function QueueSection({
isMyTurn={isMyTurn} isMyTurn={isMyTurn}
onUpdate={onAutodraftUpdate} onUpdate={onAutodraftUpdate}
/> />
{/* Notification Settings */}
<NotificationSettings
enabled={notificationsEnabled}
onEnabledChange={onNotificationsEnabledChange}
isSupported={notificationsSupported}
permissionState={notificationsPermission}
/>
</div> </div>
); );
} }

View file

@ -11,7 +11,7 @@ export function useDraftNotifications(seasonId: string) {
>("unsupported"); >("unsupported");
const [enabled, setEnabledState] = useState(false); const [enabled, setEnabledState] = useState(false);
// Check browser support and restore persisted preference // Check browser support, restore persisted preference, and watch for permission changes
useEffect(() => { useEffect(() => {
if (typeof window === "undefined" || !("Notification" in window)) { if (typeof window === "undefined" || !("Notification" in window)) {
return; return;
@ -27,6 +27,30 @@ export function useDraftNotifications(seasonId: string) {
setEnabledState(true); 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]); }, [seasonId]);
const setEnabled = useCallback( const setEnabled = useCallback(
@ -61,6 +85,7 @@ export function useDraftNotifications(seasonId: string) {
!enabled || !enabled ||
!isSupported || !isSupported ||
Notification.permission !== "granted" || Notification.permission !== "granted" ||
typeof document === "undefined" ||
!document.hidden !document.hidden
) { ) {
return; return;

18
app/lib/draft-order.ts Normal file
View file

@ -0,0 +1,18 @@
/**
* Returns the draft slot for a given pick number in a snake draft.
*/
export function getTeamForPick<T extends { draftOrder: number }>(
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);
}

View file

@ -3,7 +3,7 @@ import { eq, and, asc, desc, inArray } from "drizzle-orm";
import { database } from "~/database/context"; import { database } from "~/database/context";
import * as schema from "~/database/schema"; import * as schema from "~/database/schema";
import { useDraftSocket } from "~/hooks/useDraftSocket"; 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 { Card } from "~/components/ui/card";
import { Badge } from "~/components/ui/badge"; import { Badge } from "~/components/ui/badge";
import { Button } from "~/components/ui/button"; import { Button } from "~/components/ui/button";
@ -18,7 +18,9 @@ import { SidebarRecentPicks } from "~/components/draft/SidebarRecentPicks";
import { DraftGridSection } from "~/components/draft/DraftGridSection"; import { DraftGridSection } from "~/components/draft/DraftGridSection";
import { ConnectionOverlay } from "~/components/draft/ConnectionOverlay"; import { ConnectionOverlay } from "~/components/draft/ConnectionOverlay";
import { calculateDraftEligibility } from "~/lib/draft-eligibility"; import { calculateDraftEligibility } from "~/lib/draft-eligibility";
import { getTeamForPick } from "~/lib/draft-order";
import { useDraftNotifications } from "~/hooks/useDraftNotifications"; import { useDraftNotifications } from "~/hooks/useDraftNotifications";
import { NotificationSettings } from "~/components/NotificationSettings";
import { toast } from "sonner"; import { toast } from "sonner";
export async function loader(args: any) { export async function loader(args: any) {
@ -200,12 +202,18 @@ export default function DraftRoom() {
} = useLoaderData<typeof loader>(); } = useLoaderData<typeof loader>();
const { isConnected, connectionError, isReconnecting, on, off } = useDraftSocket(season.id, userTeam?.id); const { isConnected, connectionError, isReconnecting, on, off } = useDraftSocket(season.id, userTeam?.id);
const { const {
isSupported: notificationsSupported,
permissionState: notificationsPermission, permissionState: notificationsPermission,
enabled: notificationsEnabled, enabled: notificationsEnabled,
setEnabled: setNotificationsEnabled, setEnabled: setNotificationsEnabled,
sendNotification, sendNotification,
} = useDraftNotifications(season.id); } = 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 [picks, setPicks] = useState(draftPicks);
const [currentPick, setCurrentPick] = useState(season.currentPickNumber || 1); const [currentPick, setCurrentPick] = useState(season.currentPickNumber || 1);
const [searchQuery, setSearchQuery] = useState(""); const [searchQuery, setSearchQuery] = useState("");
@ -369,31 +377,22 @@ export default function DraftRoom() {
setPicks((prev: any) => [...prev, data.pick]); setPicks((prev: any) => [...prev, data.pick]);
setCurrentPick(data.nextPickNumber); setCurrentPick(data.nextPickNumber);
// Send browser notification // No meaningful "next turn" notification once the draft is over
const pickerName = data.pick?.team?.name || "A team"; if (data.isDraftComplete) return;
const participantName = data.pick?.participant?.name || "a participant";
const notifTitle = `${season.league.name} Draft`;
// Check if the next pick is the current user's turn const notifTitle = `${season.league.name} Draft`;
const nextPick = data.nextPickNumber; const nextSlot = getTeamForPick(data.nextPickNumber, draftSlots);
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 isNowMyTurn = !!( const isNowMyTurn = !!(
userTeam && nextSlot && nextSlot.team.id === userTeam.id userTeam && nextSlot && nextSlot.team.id === userTeam.id
); );
if (isNowMyTurn) { if (isNowMyTurn) {
sendNotification(notifTitle, `It's your turn to pick!`); sendNotificationRef.current(notifTitle, `It's your turn to pick!`);
} else { } else if (data.pick?.team?.id !== userTeam?.id) {
sendNotification( // 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, notifTitle,
`${pickerName} picked ${participantName}` `${pickerName} picked ${participantName}`
); );
@ -485,7 +484,7 @@ export default function DraftRoom() {
off("connected-teams-list", handleConnectedTeamsList); off("connected-teams-list", handleConnectedTeamsList);
off("participant-removed-from-queues", handleParticipantRemovedFromQueues); 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 // Persist sidebar collapsed state
useEffect(() => { useEffect(() => {
@ -695,16 +694,7 @@ export default function DraftRoom() {
const currentRound = Math.ceil(currentPick / draftSlots.length); const currentRound = Math.ceil(currentPick / draftSlots.length);
// Determine whose turn it is // Determine whose turn it is
const totalTeams = draftSlots.length; const currentDraftSlot = getTeamForPick(currentPick, draftSlots);
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 isMyTurn = !!( const isMyTurn = !!(
userTeam && currentDraftSlot && currentDraftSlot.team.id === userTeam.id userTeam && currentDraftSlot && currentDraftSlot.team.id === userTeam.id
); );
@ -915,15 +905,18 @@ export default function DraftRoom() {
setUserAutodraft({ isEnabled, mode }); setUserAutodraft({ isEnabled, mode });
}} }}
onReorder={handleReorderQueue} onReorder={handleReorderQueue}
notificationsEnabled={notificationsEnabled}
onNotificationsEnabledChange={setNotificationsEnabled}
notificationsSupported={notificationsSupported}
notificationsPermission={notificationsPermission}
/> />
} }
recentPicksSection={ recentPicksSection={
<SidebarRecentPicks picks={picks} /> <SidebarRecentPicks picks={picks} />
} }
settingsSection={
<NotificationSettings
enabled={notificationsEnabled}
onEnabledChange={setNotificationsEnabled}
permissionState={notificationsPermission}
/>
}
/> />
)} )}