Fix code review issues in commissioner management

- Add success/error feedback banners to commissioner add/remove actions
- Gate allUsers query to admin-only (prevents exposing all users to non-admin commissioners)
- Prevent commissioner self-removal in both action (server) and UI (client)
- Add "(you)" label next to current user in commissioner list
- Remove all unnecessary `any` type casts in favor of inferred types

https://claude.ai/code/session_01NSRMSYtb7jSFbngDS8okn3
This commit is contained in:
Claude 2026-02-20 16:19:34 +00:00
parent e1114e1d1a
commit a8ab5b6fd3
No known key found for this signature in database

View file

@ -98,8 +98,8 @@ export async function loader(args: Route.LoaderArgs) {
// Check if user is admin // Check if user is admin
const isAdmin = await isUserAdminByClerkId(userId); const isAdmin = await isUserAdminByClerkId(userId);
// Get all users (for commissioner and team assignment dropdowns) // Get all users (only for admins - used in team assignment dropdown)
const allUsers = await findAllUsers(); const allUsers = isAdmin ? await findAllUsers() : [];
// Get commissioners for this league with user info // Get commissioners for this league with user info
const commissioners = await findCommissionersByLeagueId(leagueId); const commissioners = await findCommissionersByLeagueId(leagueId);
@ -144,6 +144,7 @@ export async function loader(args: Route.LoaderArgs) {
allUsers, allUsers,
ownerMap: Object.fromEntries(ownerMap), ownerMap: Object.fromEntries(ownerMap),
commissioners: commissionerUserData, commissioners: commissionerUserData,
currentUserId: userId,
}; };
} }
@ -519,6 +520,11 @@ export async function action(args: Route.ActionArgs) {
return { error: "User is required" }; return { error: "User is required" };
} }
// Prevent self-removal
if (commissionerUserId === userId) {
return { error: "You cannot remove yourself as a commissioner" };
}
// Prevent removing the last commissioner // Prevent removing the last commissioner
const count = await countCommissionersByLeagueId(leagueId); const count = await countCommissionersByLeagueId(leagueId);
if (count <= 1) { if (count <= 1) {
@ -538,7 +544,7 @@ export async function action(args: Route.ActionArgs) {
} }
export default function LeagueSettings({ loaderData, actionData }: Route.ComponentProps) { export default function LeagueSettings({ loaderData, actionData }: Route.ComponentProps) {
const { league, season, teams, teamCount, teamsWithOwners, allSportsSeasons, draftSlots, isAdmin, allUsers, ownerMap, commissioners } = loaderData; const { league, season, teams, teamCount, teamsWithOwners, allSportsSeasons, draftSlots, isAdmin, allUsers, ownerMap, commissioners, currentUserId } = loaderData;
const navigation = useNavigation(); const navigation = useNavigation();
const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false); const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false);
const [selectedSports, setSelectedSports] = useState<Set<string>>( const [selectedSports, setSelectedSports] = useState<Set<string>>(
@ -546,8 +552,8 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
); );
const [draftOrderTeams, setDraftOrderTeams] = useState<string[]>( const [draftOrderTeams, setDraftOrderTeams] = useState<string[]>(
draftSlots.length > 0 draftSlots.length > 0
? draftSlots.map((slot: any) => slot.teamId) ? draftSlots.map((slot) => slot.teamId)
: teams.map((team: any) => team.id) : teams.map((team) => team.id)
); );
const [draftRounds, setDraftRounds] = useState<number>(season?.draftRounds || 20); const [draftRounds, setDraftRounds] = useState<number>(season?.draftRounds || 20);
const [draftDate, setDraftDate] = useState<Date | undefined>( const [draftDate, setDraftDate] = useState<Date | undefined>(
@ -562,8 +568,8 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
// Update draft order when loader data changes (after randomization) // Update draft order when loader data changes (after randomization)
useEffect(() => { useEffect(() => {
const newOrder = draftSlots.length > 0 const newOrder = draftSlots.length > 0
? draftSlots.map((slot: any) => slot.teamId) ? draftSlots.map((slot) => slot.teamId)
: teams.map((team: any) => team.id); : teams.map((team) => team.id);
setDraftOrderTeams(newOrder); setDraftOrderTeams(newOrder);
}, [draftSlots, teams]); }, [draftSlots, teams]);
@ -602,7 +608,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
}; };
const getTeamById = (teamId: string) => { const getTeamById = (teamId: string) => {
return teams.find((t: any) => t.id === teamId); return teams.find((t) => t.id === teamId);
}; };
return ( return (
@ -1035,7 +1041,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
</CardHeader> </CardHeader>
<CardContent> <CardContent>
<div className="space-y-3"> <div className="space-y-3">
{teams.map((team: any) => { {teams.map((team) => {
const ownerName = team.ownerId ? ownerMap[team.ownerId] : null; const ownerName = team.ownerId ? ownerMap[team.ownerId] : null;
return ( return (
<div <div
@ -1076,9 +1082,8 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
<SelectValue placeholder="Select user" /> <SelectValue placeholder="Select user" />
</SelectTrigger> </SelectTrigger>
<SelectContent> <SelectContent>
{allUsers.map((user: any) => { {allUsers.map((user) => {
// Check if user already owns a team in this league const userOwnsTeam = teams.some((t) => t.ownerId === user.clerkId);
const userOwnsTeam = teams.some((t: any) => t.ownerId === user.clerkId);
return ( return (
<SelectItem <SelectItem
key={user.id} key={user.id}
@ -1119,19 +1124,35 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
</CardDescription> </CardDescription>
</CardHeader> </CardHeader>
<CardContent className="space-y-4"> <CardContent className="space-y-4">
{actionData?.error && !actionData?.success && (
<div className="bg-destructive/15 text-destructive px-4 py-3 rounded-md text-sm">
{actionData.error}
</div>
)}
{actionData?.success && actionData?.message && (
<div className="bg-green-500/15 text-green-700 dark:text-green-400 px-4 py-3 rounded-md text-sm">
{actionData.message}
</div>
)}
<div className="space-y-3"> <div className="space-y-3">
{commissioners.map((commissioner: any) => ( {commissioners.map((commissioner) => (
<div <div
key={commissioner.id} key={commissioner.id}
className="flex items-center justify-between p-3 border rounded-md" className="flex items-center justify-between p-3 border rounded-md"
> >
<div className="flex-1"> <div className="flex-1">
<p className="font-medium">{commissioner.userName}</p> <p className="font-medium">
{!teams.some((t: any) => t.ownerId === commissioner.userId) && ( {commissioner.userName}
{commissioner.userId === currentUserId && (
<span className="text-xs text-muted-foreground ml-2">(you)</span>
)}
</p>
{!teams.some((t) => t.ownerId === commissioner.userId) && (
<p className="text-xs text-muted-foreground">No team in current season</p> <p className="text-xs text-muted-foreground">No team in current season</p>
)} )}
</div> </div>
{commissioners.length > 1 && ( {commissioners.length > 1 && commissioner.userId !== currentUserId && (
<Form method="post"> <Form method="post">
<input type="hidden" name="intent" value="remove-commissioner" /> <input type="hidden" name="intent" value="remove-commissioner" />
<input type="hidden" name="commissionerUserId" value={commissioner.userId} /> <input type="hidden" name="commissionerUserId" value={commissioner.userId} />
@ -1149,6 +1170,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
))} ))}
</div> </div>
{isAdmin && (
<div className="border-t pt-4"> <div className="border-t pt-4">
<p className="text-sm font-medium mb-3">Add Commissioner</p> <p className="text-sm font-medium mb-3">Add Commissioner</p>
<Form method="post" className="flex gap-2"> <Form method="post" className="flex gap-2">
@ -1158,9 +1180,9 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
<SelectValue placeholder="Select a user" /> <SelectValue placeholder="Select a user" />
</SelectTrigger> </SelectTrigger>
<SelectContent> <SelectContent>
{allUsers.map((user: any) => { {allUsers.map((user) => {
const alreadyCommissioner = commissioners.some( const alreadyCommissioner = commissioners.some(
(c: any) => c.userId === user.clerkId (c) => c.userId === user.clerkId
); );
return ( return (
<SelectItem <SelectItem
@ -1185,6 +1207,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
</Button> </Button>
</Form> </Form>
</div> </div>
)}
</CardContent> </CardContent>
</Card> </Card>