From a8ab5b6fd30df60f97b6624457563eac6bebcd4d Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Fri, 20 Feb 2026 16:19:34 +0000
Subject: [PATCH] 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
---
 app/routes/leagues/$leagueId.settings.tsx | 135 +++++++++++++---------
 1 file changed, 79 insertions(+), 56 deletions(-)

diff --git a/app/routes/leagues/$leagueId.settings.tsx b/app/routes/leagues/$leagueId.settings.tsx
index 2b1b702..7b181ea 100644
--- a/app/routes/leagues/$leagueId.settings.tsx
+++ b/app/routes/leagues/$leagueId.settings.tsx
@@ -98,8 +98,8 @@ export async function loader(args: Route.LoaderArgs) {
   // Check if user is admin
   const isAdmin = await isUserAdminByClerkId(userId);
 
-  // Get all users (for commissioner and team assignment dropdowns)
-  const allUsers = await findAllUsers();
+  // Get all users (only for admins - used in team assignment dropdown)
+  const allUsers = isAdmin ? await findAllUsers() : [];
 
   // Get commissioners for this league with user info
   const commissioners = await findCommissionersByLeagueId(leagueId);
@@ -144,6 +144,7 @@ export async function loader(args: Route.LoaderArgs) {
     allUsers,
     ownerMap: Object.fromEntries(ownerMap),
     commissioners: commissionerUserData,
+    currentUserId: userId,
   };
 }
 
@@ -519,6 +520,11 @@ export async function action(args: Route.ActionArgs) {
       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
     const count = await countCommissionersByLeagueId(leagueId);
     if (count <= 1) {
@@ -538,16 +544,16 @@ export async function action(args: Route.ActionArgs) {
 }
 
 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 [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false);
   const [selectedSports, setSelectedSports] = useState<Set<string>>(
     new Set(season?.seasonSports?.map((s: any) => s.sportsSeason.id) || [])
   );
   const [draftOrderTeams, setDraftOrderTeams] = useState<string[]>(
-    draftSlots.length > 0 
-      ? draftSlots.map((slot: any) => slot.teamId)
-      : teams.map((team: any) => team.id)
+    draftSlots.length > 0
+      ? draftSlots.map((slot) => slot.teamId)
+      : teams.map((team) => team.id)
   );
   const [draftRounds, setDraftRounds] = useState<number>(season?.draftRounds || 20);
   const [draftDate, setDraftDate] = useState<Date | undefined>(
@@ -561,9 +567,9 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
 
   // Update draft order when loader data changes (after randomization)
   useEffect(() => {
-    const newOrder = draftSlots.length > 0 
-      ? draftSlots.map((slot: any) => slot.teamId)
-      : teams.map((team: any) => team.id);
+    const newOrder = draftSlots.length > 0
+      ? draftSlots.map((slot) => slot.teamId)
+      : teams.map((team) => team.id);
     setDraftOrderTeams(newOrder);
   }, [draftSlots, teams]);
 
@@ -602,7 +608,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
   };
 
   const getTeamById = (teamId: string) => {
-    return teams.find((t: any) => t.id === teamId);
+    return teams.find((t) => t.id === teamId);
   };
 
   return (
@@ -1035,7 +1041,7 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
           </CardHeader>
           <CardContent>
             <div className="space-y-3">
-              {teams.map((team: any) => {
+              {teams.map((team) => {
                 const ownerName = team.ownerId ? ownerMap[team.ownerId] : null;
                 return (
                   <div
@@ -1076,12 +1082,11 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
                               <SelectValue placeholder="Select user" />
                             </SelectTrigger>
                             <SelectContent>
-                              {allUsers.map((user: any) => {
-                                // Check if user already owns a team in this league
-                                const userOwnsTeam = teams.some((t: any) => t.ownerId === user.clerkId);
+                              {allUsers.map((user) => {
+                                const userOwnsTeam = teams.some((t) => t.ownerId === user.clerkId);
                                 return (
-                                  <SelectItem 
-                                    key={user.id} 
+                                  <SelectItem
+                                    key={user.id}
                                     value={user.clerkId}
                                     disabled={userOwnsTeam}
                                   >
@@ -1119,19 +1124,35 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
             </CardDescription>
           </CardHeader>
           <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">
-              {commissioners.map((commissioner: any) => (
+              {commissioners.map((commissioner) => (
                 <div
                   key={commissioner.id}
                   className="flex items-center justify-between p-3 border rounded-md"
                 >
                   <div className="flex-1">
-                    <p className="font-medium">{commissioner.userName}</p>
-                    {!teams.some((t: any) => t.ownerId === commissioner.userId) && (
+                    <p className="font-medium">
+                      {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>
                     )}
                   </div>
-                  {commissioners.length > 1 && (
+                  {commissioners.length > 1 && commissioner.userId !== currentUserId && (
                     <Form method="post">
                       <input type="hidden" name="intent" value="remove-commissioner" />
                       <input type="hidden" name="commissionerUserId" value={commissioner.userId} />
@@ -1149,42 +1170,44 @@ export default function LeagueSettings({ loaderData, actionData }: Route.Compone
               ))}
             </div>
 
-            <div className="border-t pt-4">
-              <p className="text-sm font-medium mb-3">Add Commissioner</p>
-              <Form method="post" className="flex gap-2">
-                <input type="hidden" name="intent" value="add-commissioner" />
-                <Select name="userClerkId" required>
-                  <SelectTrigger className="flex-1">
-                    <SelectValue placeholder="Select a user" />
-                  </SelectTrigger>
-                  <SelectContent>
-                    {allUsers.map((user: any) => {
-                      const alreadyCommissioner = commissioners.some(
-                        (c: any) => c.userId === user.clerkId
-                      );
-                      return (
-                        <SelectItem
-                          key={user.id}
-                          value={user.clerkId}
-                          disabled={alreadyCommissioner}
-                        >
-                          {user.username || user.displayName || user.email}
-                          {alreadyCommissioner && " (already commissioner)"}
-                        </SelectItem>
-                      );
-                    })}
-                  </SelectContent>
-                </Select>
-                <Button
-                  type="submit"
-                  variant="outline"
-                  size="sm"
-                  disabled={navigation.state === "submitting"}
-                >
-                  Add
-                </Button>
-              </Form>
-            </div>
+            {isAdmin && (
+              <div className="border-t pt-4">
+                <p className="text-sm font-medium mb-3">Add Commissioner</p>
+                <Form method="post" className="flex gap-2">
+                  <input type="hidden" name="intent" value="add-commissioner" />
+                  <Select name="userClerkId" required>
+                    <SelectTrigger className="flex-1">
+                      <SelectValue placeholder="Select a user" />
+                    </SelectTrigger>
+                    <SelectContent>
+                      {allUsers.map((user) => {
+                        const alreadyCommissioner = commissioners.some(
+                          (c) => c.userId === user.clerkId
+                        );
+                        return (
+                          <SelectItem
+                            key={user.id}
+                            value={user.clerkId}
+                            disabled={alreadyCommissioner}
+                          >
+                            {user.username || user.displayName || user.email}
+                            {alreadyCommissioner && " (already commissioner)"}
+                          </SelectItem>
+                        );
+                      })}
+                    </SelectContent>
+                  </Select>
+                  <Button
+                    type="submit"
+                    variant="outline"
+                    size="sm"
+                    disabled={navigation.state === "submitting"}
+                  >
+                    Add
+                  </Button>
+                </Form>
+              </div>
+            )}
           </CardContent>
         </Card>