chrisp/brackt
1
1
Fork 0
Code Issues 42 Pull requests Projects Releases Packages Wiki Activity Actions

Fix all code review issues in pre-draft rankings feature

Browse source 
- Replace useFetcher with direct fetch() for add/remove/reorder so rapid
  clicks no longer cancel in-flight requests
- Add toast.error() on all failure paths (matching live draft room pattern)
  and revert optimistic state when operations fail
- Add useRef to give handleReorder a stable reference without a localQueue
  closure dependency, preventing unnecessary QueueSection re-renders
- Convert allPlayersPanel and rankingsPanel to useMemo
- Remove leagueId from loader return; read from useParams() instead
- Extract queueBuilderHref to a variable in the league home component
- Add emptyMessage prop to QueueSection with a correct message for the
  pre-draft context ("Add players from All Players...")
- Add draft-queue-access.test.ts covering all loader access control paths:
  401/403 errors, status-based redirects, and redirect URL correctness

https://claude.ai/code/session_01Gu2DkTWL3nv74EMuGPpxhG
This commit is contained in:
Claude 2026-05-15 23:53:34 +00:00
parent fb392c4fdd
commit 24d9ce2f31
No known key found for this signature in database
4 changed files with 350 additions and 89 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/components/draft/QueueSection.tsx
View file

@ -34,6 +34,7 @@ interface QueueSectionProps {
onRemoveFromQueue: (queueId: string) => void;
onReorder: (participantIds: string[]) => void;
onMakePick?: (participantId: string) => void;
emptyMessage?: string;
}
// Sortable queue item component
@ -125,6 +126,7 @@ export const QueueSection = memo(function QueueSection({
onRemoveFromQueue,
onReorder,
onMakePick,
emptyMessage = "Click participants in Available to add to your queue",
}: QueueSectionProps) {
const sensors = useSensors(
useSensor(PointerSensor, {
@ -160,7 +162,7 @@ export const QueueSection = memo(function QueueSection({
{/* Queue List */}
{queue.length === 0 ? (
<p className="text-muted-foreground text-sm text-center py-8">
Click participants in Available to add to your queue
{emptyMessage}
</p>
) : (
<DndContext

216
app/routes/leagues/$leagueId.draft-queue.$seasonId.tsx
View file

@ -1,6 +1,7 @@
import { redirect, useFetcher, useLoaderData, useRevalidator, Link } from "react-router";
import { useCallback, useEffect, useMemo, useState } from "react";
import { redirect, useLoaderData, useRevalidator, Link, useParams } from "react-router";
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
import { ArrowLeft, ListOrdered, Users } from "lucide-react";
import { toast } from "sonner";
import { auth } from "~/lib/auth.server";
import { findSeasonWithTeamsAndLeague } from "~/models/season";
import { getDraftParticipants } from "~/models/season-participant";
@ -55,7 +56,6 @@ export async function loader(args: Route.LoaderArgs) {
return {
season,
leagueId,
userTeam,
availableParticipants,
userQueue,
@ -65,8 +65,9 @@ export async function loader(args: Route.LoaderArgs) {
type QueueItem = { id: string; participantId: string };
export default function PreDraftRankings() {
const { season, leagueId, userTeam, availableParticipants, userQueue } =
const { season, userTeam, availableParticipants, userQueue } =
useLoaderData<typeof loader>();
const { leagueId } = useParams<{ leagueId: string }>();
const { revalidate } = useRevalidator();
const [localQueue, setLocalQueue] = useState<QueueItem[]>(userQueue);
@ -76,16 +77,11 @@ export default function PreDraftRankings() {
setLocalQueue(userQueue);
}, [userQueue]);
const addFetcher = useFetcher<{ success: boolean }>();
const removeFetcher = useFetcher();
const reorderFetcher = useFetcher();
// After add completes, revalidate to replace the optimistic temp ID with the real one
// Always-current ref so reorder callback stays stable (no localQueue dependency)
const queueRef = useRef(localQueue);
useEffect(() => {
if (addFetcher.state === "idle" && addFetcher.data?.success) {
revalidate();
}
}, [addFetcher.state, addFetcher.data, revalidate]);
queueRef.current = localQueue;
}, [localQueue]);
const queuedParticipantIds = useMemo(
() => new Set(localQueue.map((q) => q.participantId)),
@ -93,123 +89,169 @@ export default function PreDraftRankings() {
);
const handleAdd = useCallback(
(participantId: string) => {
async (participantId: string) => {
if (queuedParticipantIds.has(participantId)) return;
const tempId = `temp-${Date.now()}-${participantId}`;
setLocalQueue((prev) => [...prev, { id: tempId, participantId }]);
const formData = new FormData();
formData.append("seasonId", season.id);
formData.append("teamId", userTeam.id);
formData.append("participantId", participantId);
addFetcher.submit(formData, { method: "POST", action: "/api/queue/add" });
try {
const res = await fetch("/api/queue/add", { method: "POST", body: formData });
const data = await res.json();
if (data.error) {
setLocalQueue((prev) => prev.filter((item) => item.id !== tempId));
toast.error(data.error || "Failed to add to rankings");
return;
}
revalidate(); // swap temp ID for the real server-assigned ID
} catch {
setLocalQueue((prev) => prev.filter((item) => item.id !== tempId));
toast.error("Network error — failed to add to rankings");
}
},
[queuedParticipantIds, season.id, userTeam.id, addFetcher]
[queuedParticipantIds, season.id, userTeam.id, revalidate]
);
const handleRemove = useCallback(
(queueId: string) => {
async (queueId: string) => {
setLocalQueue((prev) => prev.filter((item) => item.id !== queueId));
const formData = new FormData();
formData.append("queueId", queueId);
formData.append("teamId", userTeam.id);
removeFetcher.submit(formData, { method: "POST", action: "/api/queue/remove" });
try {
const res = await fetch("/api/queue/remove", { method: "POST", body: formData });
const data = await res.json();
if (data.error) {
toast.error(data.error || "Failed to remove from rankings");
revalidate();
}
} catch {
toast.error("Network error — failed to remove from rankings");
revalidate();
}
},
[userTeam.id, removeFetcher]
[userTeam.id, revalidate]
);
// Remove by participantId — used from the All Players list
const handleRemoveByParticipantId = useCallback(
(participantId: string) => {
const item = localQueue.find((q) => q.participantId === participantId);
const item = queueRef.current.find((q) => q.participantId === participantId);
if (item) handleRemove(item.id);
},
[localQueue, handleRemove]
[handleRemove]
);
const handleReorder = useCallback(
(participantIds: string[]) => {
async (participantIds: string[]) => {
const previousQueue = queueRef.current;
setLocalQueue((prev) => {
const byParticipantId = new Map(prev.map((item) => [item.participantId, item]));
return participantIds
.map((pid) => byParticipantId.get(pid))
.filter((item): item is (typeof prev)[number] => item !== undefined);
.filter((item): item is QueueItem => item !== undefined);
});
const formData = new FormData();
formData.append("teamId", userTeam.id);
formData.append("seasonId", season.id);
formData.append("participantIds", JSON.stringify(participantIds));
reorderFetcher.submit(formData, { method: "POST", action: "/api/queue/reorder" });
try {
const res = await fetch("/api/queue/reorder", { method: "POST", body: formData });
const data = await res.json();
if (data.error) {
setLocalQueue(previousQueue);
toast.error(data.error || "Failed to reorder rankings");
}
} catch {
setLocalQueue(previousQueue);
toast.error("Network error — failed to reorder rankings");
}
},
[userTeam.id, season.id, reorderFetcher]
[userTeam.id, season.id]
);
// ── Shared panel content ────────────────────────────────────────────────────
const allPlayersPanel = (
<div className="space-y-1">
{availableParticipants.map((p) => {
const inQueue = queuedParticipantIds.has(p.id);
return (
<div
key={p.id}
className="flex items-center gap-3 rounded-lg bg-muted/50 px-3 py-2"
>
<div className="min-w-0 flex-1">
<p className="font-medium text-sm truncate">{p.name}</p>
<p className="text-xs text-muted-foreground">{p.sport.name}</p>
const allPlayersPanel = useMemo(
() => (
<div className="space-y-1">
{availableParticipants.map((p) => {
const inQueue = queuedParticipantIds.has(p.id);
return (
<div
key={p.id}
className="flex items-center gap-3 rounded-lg bg-muted/50 px-3 py-2"
>
<div className="min-w-0 flex-1">
<p className="font-medium text-sm truncate">{p.name}</p>
<p className="text-xs text-muted-foreground">{p.sport.name}</p>
</div>
{p.vorpValue != null && (
<span className="text-xs text-muted-foreground tabular-nums shrink-0">
{Number(p.vorpValue).toFixed(1)}
</span>
)}
{inQueue ? (
<Button
size="sm"
variant="ghost"
onClick={() => handleRemoveByParticipantId(p.id)}
className="h-7 text-xs shrink-0 text-destructive hover:text-destructive hover:bg-destructive/10"
>
Remove
</Button>
) : (
<Button
size="sm"
variant="outline"
onClick={() => handleAdd(p.id)}
className="h-7 text-xs shrink-0"
>
+ Add
</Button>
)}
</div>
{p.vorpValue != null && (
<span className="text-xs text-muted-foreground tabular-nums shrink-0">
{Number(p.vorpValue).toFixed(1)}
</span>
)}
{inQueue ? (
<Button
size="sm"
variant="ghost"
onClick={() => handleRemoveByParticipantId(p.id)}
className="h-7 text-xs shrink-0 text-destructive hover:text-destructive hover:bg-destructive/10"
>
Remove
</Button>
) : (
<Button
size="sm"
variant="outline"
onClick={() => handleAdd(p.id)}
className="h-7 text-xs shrink-0"
>
+ Add
</Button>
)}
</div>
);
})}
{availableParticipants.length === 0 && (
<p className="text-sm text-muted-foreground text-center py-12">
No participants have been added to this season yet.
</p>
)}
</div>
);
})}
{availableParticipants.length === 0 && (
<p className="text-sm text-muted-foreground text-center py-12">
No participants have been added to this season yet.
</p>
)}
</div>
),
[availableParticipants, queuedParticipantIds, handleAdd, handleRemoveByParticipantId]
);
const rankingsPanel = (
<>
<div className="rounded-lg border bg-card">
<QueueSection
queue={localQueue}
availableParticipants={availableParticipants}
canPick={false}
onRemoveFromQueue={handleRemove}
onReorder={handleReorder}
/>
</div>
{localQueue.length > 0 && (
<p className="text-xs text-muted-foreground mt-2 text-center">
Drag to reorder · Autopick follows this order
</p>
)}
</>
const rankingsPanel = useMemo(
() => (
<>
<div className="rounded-lg border bg-card">
<QueueSection
queue={localQueue}
availableParticipants={availableParticipants}
canPick={false}
onRemoveFromQueue={handleRemove}
onReorder={handleReorder}
emptyMessage="Add players from All Players to set your ranking order."
/>
</div>
{localQueue.length > 0 && (
<p className="text-xs text-muted-foreground mt-2 text-center">
Drag to reorder · Autopick follows this order
</p>
)}
</>
),
[localQueue, availableParticipants, handleRemove, handleReorder]
);
// ── Render ──────────────────────────────────────────────────────────────────

6
app/routes/leagues/$leagueId.tsx
View file

@ -210,7 +210,11 @@ export default function LeagueHome({ loaderData }: Route.ComponentProps) {
draftTimezone={currentUserTimezone ?? season.overnightPauseTimezone}
draftBoardHref={`/leagues/${league.id}/draft-board/${season.id}`}
draftRoomHref={`/leagues/${league.id}/draft/${season.id}`}
queueBuilderHref={myTeam && season.status === "pre_draft" ? `/leagues/${league.id}/draft-queue/${season.id}` : undefined}
queueBuilderHref={
myTeam && season.status === "pre_draft"
? `/leagues/${league.id}/draft-queue/${season.id}`
: undefined
}
userDraftPosition={userDraftPosition}
overnightPauseMode={season.overnightPauseMode}
overnightPauseStart={season.overnightPauseStart}

213
app/routes/leagues/__tests__/draft-queue-access.test.ts Normal file
View file

@ -0,0 +1,213 @@
import { describe, it, expect } from 'vitest';
// ---------------------------------------------------------------------------
// Helpers
// ---------------------------------------------------------------------------
const createMockSeason = (overrides: Partial<{
id: string;
leagueId: string;
status: 'pre_draft' | 'draft' | 'active' | 'completed';
}> = {}) => ({
id: 'season-1',
leagueId: 'league-1',
year: 2025,
status: 'pre_draft' as const,
templateId: null,
draftRounds: 20,
flexSpots: 0,
draftDateTime: null,
draftInitialTime: 120,
draftIncrementTime: 15,
currentPickNumber: null,
draftStartedAt: null,
draftPaused: false,
inviteCode: 'TEST123',
pointsFor1st: 100,
pointsFor2nd: 70,
pointsFor3rd: 50,
pointsFor4th: 40,
pointsFor5th: 25,
pointsFor6th: 25,
pointsFor7th: 15,
pointsFor8th: 15,
createdAt: new Date('2025-01-01'),
updatedAt: new Date('2025-01-01'),
...overrides,
});
type MockSeason = ReturnType<typeof createMockSeason>;
// Mirrors the access-control and redirect logic from the pre-draft rankings loader.
// Returns null for a successful load, a redirect URL, or an error object.
function checkPreDraftRankingsAccess(opts: {
leagueId: string;
season: MockSeason;
userId: string | null;
hasTeam: boolean;
}): { redirect: string } | { status: number; message: string } | null {
const { leagueId, season, userId, hasTeam } = opts;
const { id: seasonId } = season;
if (season.status === 'draft') {
return { redirect: `/leagues/${leagueId}/draft/${seasonId}` };
}
if (season.status === 'active' || season.status === 'completed') {
return { redirect: `/leagues/${leagueId}/draft-board/${seasonId}` };
}
if (!userId) {
return { status: 401, message: 'You must be logged in to set pre-draft rankings' };
}
if (!hasTeam) {
return { status: 403, message: 'You do not have a team in this season' };
}
return null;
}
// ---------------------------------------------------------------------------
// Redirect behaviour based on season status
// ---------------------------------------------------------------------------
describe('Pre-Draft Rankings Access - Season Status Redirects', () => {
it('redirects to the draft room when status is draft', () => {
const season = createMockSeason({ status: 'draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toEqual({ redirect: '/leagues/league-1/draft/season-1' });
});
it('redirects to the draft board when status is active', () => {
const season = createMockSeason({ status: 'active' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toEqual({ redirect: '/leagues/league-1/draft-board/season-1' });
});
it('redirects to the draft board when status is completed', () => {
const season = createMockSeason({ status: 'completed' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toEqual({ redirect: '/leagues/league-1/draft-board/season-1' });
});
it('does not redirect when status is pre_draft', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toBeNull();
});
});
// ---------------------------------------------------------------------------
// Authentication
// ---------------------------------------------------------------------------
describe('Pre-Draft Rankings Access - Authentication', () => {
it('returns 401 when no user is logged in', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: null,
hasTeam: false,
});
expect(result).toMatchObject({ status: 401 });
});
it('uses a descriptive 401 message', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: null,
hasTeam: false,
});
expect(result).toMatchObject({ message: 'You must be logged in to set pre-draft rankings' });
});
});
// ---------------------------------------------------------------------------
// Team membership
// ---------------------------------------------------------------------------
describe('Pre-Draft Rankings Access - Team Membership', () => {
it('returns 403 for a logged-in user with no team in the season', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'outsider',
hasTeam: false,
});
expect(result).toMatchObject({ status: 403 });
});
it('uses a descriptive 403 message', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'outsider',
hasTeam: false,
});
expect(result).toMatchObject({ message: 'You do not have a team in this season' });
});
it('grants access to a logged-in user who owns a team', () => {
const season = createMockSeason({ status: 'pre_draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'owner-user',
hasTeam: true,
});
expect(result).toBeNull();
});
});
// ---------------------------------------------------------------------------
// Redirect targets use the correct leagueId and seasonId
// ---------------------------------------------------------------------------
describe('Pre-Draft Rankings Access - Redirect URL Correctness', () => {
it('uses the leagueId from params in the redirect URL', () => {
const season = createMockSeason({ id: 'season-99', status: 'draft' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-42',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toEqual({ redirect: '/leagues/league-42/draft/season-99' });
});
it('uses the seasonId in the redirect URL', () => {
const season = createMockSeason({ id: 'season-77', status: 'active' });
const result = checkPreDraftRankingsAccess({
leagueId: 'league-1',
season,
userId: 'user-1',
hasTeam: true,
});
expect(result).toEqual({ redirect: '/leagues/league-1/draft-board/season-77' });
});
});