Harden sport icon cleanup (#397)

app/components/ui/SportIconUploader.tsx

@@ -1,14 +1,21 @@
-import { useRef, useState } from "react";
+import { useId, useRef, useState } from "react";
 import { ImageIcon, Trash2, Upload } from "lucide-react";
 import { Button } from "~/components/ui/button";
 import { SportIcon } from "~/components/SportIcon";
 import { cn } from "~/lib/utils";
 
+interface SportIconOption {
+  id: string;
+  name: string;
+  iconUrl: string;
+}
+
 interface SportIconUploaderProps {
   name?: string;
   uploadUrl: string;
   initialIconUrl?: string | null;
   sportName?: string;
+  existingIconOptions?: SportIconOption[];
 }
 
 const MAX_UPLOAD_BYTES = 1024 * 1024;
@@ -19,7 +26,9 @@ export function SportIconUploader({
   uploadUrl,
   initialIconUrl = null,
   sportName = "Sport icon",
+  existingIconOptions = [],
 }: SportIconUploaderProps) {
+  const selectId = useId();
   const fileInputRef = useRef<HTMLInputElement | null>(null);
   const [iconUrl, setIconUrl] = useState(initialIconUrl ?? "");
   const [selectedFile, setSelectedFile] = useState<File | null>(null);
@@ -60,6 +69,11 @@ export function SportIconUploader({
     clearSelectedFile();
   }
 
+  function chooseExistingIcon(nextIconUrl: string) {
+    setIconUrl(nextIconUrl);
+    clearSelectedFile();
+  }
+
   async function uploadSelectedFile() {
     if (!selectedFile) {
       setError("Choose an SVG first.");
@@ -107,6 +121,30 @@ export function SportIconUploader({
         className="sr-only"
       />
 
+      {existingIconOptions.length > 0 && (
+        <div className="space-y-2">
+          <label htmlFor={selectId} className="text-sm font-medium">
+            Use an existing sport icon
+          </label>
+          <select
+            id={selectId}
+            value={existingIconOptions.some((option) => option.iconUrl === iconUrl) ? iconUrl : ""}
+            onChange={(event) => chooseExistingIcon(event.target.value)}
+            className="flex h-10 w-full rounded-md border border-input bg-background px-3 py-2 text-sm text-foreground ring-offset-background focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50"
+          >
+            <option value="">Choose an icon from another sport...</option>
+            {existingIconOptions.map((option) => (
+              <option key={option.id} value={option.iconUrl}>
+                {option.name}
+              </option>
+            ))}
+          </select>
+          <p className="text-sm text-muted-foreground">
+            Selecting a sport reuses its current icon URL; no upload required.
+          </p>
+        </div>
+      )}
+
       {iconUrl && !selectedFile && (
         <div className="space-y-3 rounded-md border border-border bg-muted/40 p-4">
           <div className="flex items-center gap-3">

app/components/ui/__tests__/SportIconUploader.test.tsx

@@ -28,6 +28,25 @@ describe("SportIconUploader", () => {
     expect(await screen.findByText("Choose an SVG file.")).toBeInTheDocument();
   });
 
+  it("chooses an icon from an existing sport", () => {
+    const { container } = render(
+      <SportIconUploader
+        uploadUrl="/api/upload-sport-icon"
+        existingIconOptions={[
+          { id: "sport-1", name: "NFL", iconUrl: "nfl.svg" },
+          { id: "sport-2", name: "NBA", iconUrl: "nba.svg" },
+        ]}
+      />
+    );
+
+    fireEvent.change(screen.getByRole("combobox", { name: "Use an existing sport icon" }), {
+      target: { value: "nba.svg" },
+    });
+
+    expect(container.querySelector('input[name="iconUrl"]')).toHaveValue("nba.svg");
+    expect(screen.getByText("Current icon")).toBeInTheDocument();
+  });
+
   it("uploads an SVG and updates the hidden input", async () => {
     vi.stubGlobal(
       "fetch",

app/lib/__tests__/sport-icon-url.test.ts

@@ -41,5 +41,6 @@ describe("sport icon URL validation", () => {
     expect(parseSportIconUrlInput("https://res.cloudinary.com/demo/image/upload/v123/avatars/nfl.svg")).toBeUndefined();
     expect(parseSportIconUrlInput("nfl.png")).toBeUndefined();
     expect(parseSportIconUrlInput("../nfl.svg")).toBeUndefined();
+    expect(parseSportIconUrlInput("/sports-icons/nfl.svg")).toBeUndefined();
   });
 });

app/lib/sport-icon-cleanup.server.ts

@@ -0,0 +1,28 @@
+import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server";
+import { logger } from "~/lib/logger";
+import { isSportIconUrlUsed } from "~/models/sport";
+
+interface DeleteSportIconIfUnusedArgs {
+  iconUrl: string;
+  excludeSportId?: string;
+  reason: "replaced" | "unused";
+}
+
+export async function deleteSportIconIfUnused({
+  iconUrl,
+  excludeSportId,
+  reason,
+}: DeleteSportIconIfUnusedArgs): Promise<void> {
+  try {
+    if (await isSportIconUrlUsed(iconUrl, excludeSportId)) {
+      return;
+    }
+  } catch (error) {
+    logger.error("Failed to check whether sport icon is reused; skipping Cloudinary cleanup:", error);
+    return;
+  }
+
+  deleteCloudinaryImageByUrl(iconUrl).catch((error) => {
+    logger.error(`Failed to delete ${reason} sport icon from Cloudinary:`, error);
+  });
+}

app/models/sport.ts

@@ -1,4 +1,4 @@
-import { eq, sql } from "drizzle-orm";
+import { and, eq, ne, sql } from "drizzle-orm";
 import { database } from "~/database/context";
 import * as schema from "~/database/schema";
 
@@ -48,6 +48,17 @@ export async function findAllSports(): Promise<Sport[]> {
   });
 }
 
+export async function isSportIconUrlUsed(iconUrl: string, excludeSportId?: string): Promise<boolean> {
+  const db = database();
+  const sport = await db.query.sports.findFirst({
+    where: excludeSportId
+      ? and(eq(schema.sports.iconUrl, iconUrl), ne(schema.sports.id, excludeSportId))
+      : eq(schema.sports.iconUrl, iconUrl),
+    columns: { id: true },
+  });
+  return Boolean(sport);
+}
+
 export async function findAllSportsWithActiveSeasons(): Promise<SportWithActiveSeasons[]> {
   const db = database();
   const sports = await db.query.sports.findMany({

app/routes/__tests__/admin.sports.$id.test.ts

@@ -1,7 +1,9 @@
 import { beforeEach, describe, expect, it, vi } from "vitest";
 
 vi.mock("~/models/sport", () => ({
+  findAllSports: vi.fn(),
   findSportById: vi.fn(),
+  isSportIconUrlUsed: vi.fn(),
   updateSport: vi.fn(),
 }));
 vi.mock("~/lib/cloudinary.server", () => ({
@@ -17,10 +19,10 @@ vi.mock("~/services/simulations/registry", () => ({
 
 import { action } from "../admin.sports.$id";
 import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server";
-import { findSportById, updateSport } from "~/models/sport";
+import { findSportById, isSportIconUrlUsed, updateSport } from "~/models/sport";
 
 function makeRequest(iconUrl: string) {
-  const formData = new FormData();
+  const formData = new URLSearchParams();
   formData.set("name", "NFL");
   formData.set("type", "team");
   formData.set("slug", "nfl");
@@ -30,12 +32,14 @@ function makeRequest(iconUrl: string) {
 
   return new Request("http://test/admin/sports/sport-1", {
     method: "POST",
+    headers: { "Content-Type": "application/x-www-form-urlencoded" },
     body: formData,
   });
 }
 
 beforeEach(() => {
   vi.clearAllMocks();
+  vi.mocked(isSportIconUrlUsed).mockResolvedValue(false);
   vi.mocked(findSportById).mockResolvedValue({
     id: "sport-1",
     name: "Old NFL",
@@ -105,4 +109,33 @@ describe("admin sport edit action", () => {
 
     expect(deleteCloudinaryImageByUrl).toHaveBeenCalledWith(nextIconUrl);
   });
+
+  it("does not delete a reused existing icon when update fails", async () => {
+    const nextIconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/new.svg";
+    vi.mocked(updateSport).mockRejectedValue(new Error("duplicate"));
+    vi.mocked(isSportIconUrlUsed).mockResolvedValue(true);
+
+    await action({
+      request: makeRequest(nextIconUrl),
+      params: { id: "sport-1" },
+      context: {},
+    } as never);
+
+    expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled();
+  });
+
+  it("returns the update error when icon reuse lookup fails during cleanup", async () => {
+    const nextIconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/new.svg";
+    vi.mocked(updateSport).mockRejectedValue(new Error("database unavailable"));
+    vi.mocked(isSportIconUrlUsed).mockRejectedValue(new Error("database unavailable"));
+
+    const response = await action({
+      request: makeRequest(nextIconUrl),
+      params: { id: "sport-1" },
+      context: {},
+    } as never);
+
+    expect(response).toEqual({ error: "Failed to update sport: database unavailable" });
+    expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled();
+  });
 });

app/routes/__tests__/admin.sports.new.test.ts

@@ -2,6 +2,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
 
 vi.mock("~/models/sport", () => ({
   createSport: vi.fn(),
+  findAllSports: vi.fn(),
+  isSportIconUrlUsed: vi.fn(),
 }));
 vi.mock("~/lib/cloudinary.server", () => ({
   deleteCloudinaryImageByUrl: vi.fn().mockResolvedValue(undefined),
@@ -12,10 +14,10 @@ vi.mock("~/lib/logger", () => ({
 
 import { action } from "../admin.sports.new";
 import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server";
-import { createSport } from "~/models/sport";
+import { createSport, isSportIconUrlUsed } from "~/models/sport";
 
 function makeRequest(iconUrl: string) {
-  const formData = new FormData();
+  const formData = new URLSearchParams();
   formData.set("name", "NFL");
   formData.set("type", "team");
   formData.set("slug", "nfl");
@@ -24,6 +26,7 @@ function makeRequest(iconUrl: string) {
 
   return new Request("http://test/admin/sports/new", {
     method: "POST",
+    headers: { "Content-Type": "application/x-www-form-urlencoded" },
     body: formData,
   });
 }
@@ -31,6 +34,7 @@ function makeRequest(iconUrl: string) {
 beforeEach(() => {
   vi.clearAllMocks();
   vi.mocked(createSport).mockResolvedValue({} as never);
+  vi.mocked(isSportIconUrlUsed).mockResolvedValue(false);
 });
 
 describe("admin sport create action", () => {
@@ -57,4 +61,33 @@ describe("admin sport create action", () => {
 
     expect(deleteCloudinaryImageByUrl).toHaveBeenCalledWith(iconUrl);
   });
+
+  it("does not delete a reused existing icon when create fails", async () => {
+    const iconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/nfl.svg";
+    vi.mocked(createSport).mockRejectedValue(new Error("duplicate"));
+    vi.mocked(isSportIconUrlUsed).mockResolvedValue(true);
+
+    await action({
+      request: makeRequest(iconUrl),
+      params: {},
+      context: {},
+    } as never);
+
+    expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled();
+  });
+
+  it("returns the create error when icon reuse lookup fails during cleanup", async () => {
+    const iconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/nfl.svg";
+    vi.mocked(createSport).mockRejectedValue(new Error("database unavailable"));
+    vi.mocked(isSportIconUrlUsed).mockRejectedValue(new Error("database unavailable"));
+
+    const response = await action({
+      request: makeRequest(iconUrl),
+      params: {},
+      context: {},
+    } as never);
+
+    expect(response).toEqual({ error: "Failed to create sport. The slug might already exist." });
+    expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled();
+  });
 });

app/routes/admin.sports.$id.tsx

@@ -2,8 +2,8 @@ import { Form, Link, redirect } from "react-router";
 import type { Route } from "./+types/admin.sports.$id";
 
 import { logger } from "~/lib/logger";
-import { findSportById, updateSport } from "~/models/sport";
+import { findAllSports, findSportById, updateSport } from "~/models/sport";
-import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server";
+import { deleteSportIconIfUnused } from "~/lib/sport-icon-cleanup.server";
 import { parseSportIconUrlInput } from "~/lib/sport-icon-url";
 import { Button } from "~/components/ui/button";
 import { Input } from "~/components/ui/input";
@@ -37,7 +37,14 @@ export async function loader({ params }: Route.LoaderArgs) {
     throw new Response("Sport not found", { status: 404 });
   }
   
-  return { sport };
+  const sports = await findAllSports();
+
+  return {
+    sport,
+    existingIconOptions: sports
+      .filter((option) => option.id !== sport.id && Boolean(option.iconUrl))
+      .map((option) => ({ id: option.id, name: option.name, iconUrl: option.iconUrl as string })),
+  };
 }
 
 export async function action({ request, params }: Route.ActionArgs) {
@@ -90,8 +97,10 @@ export async function action({ request, params }: Route.ActionArgs) {
     });
 
     if (existingSport?.iconUrl && existingSport.iconUrl !== updatedSport.iconUrl) {
-      deleteCloudinaryImageByUrl(existingSport.iconUrl).catch((error) => {
+      await deleteSportIconIfUnused({
-        logger.error("Failed to delete replaced sport icon from Cloudinary:", error);
+        iconUrl: existingSport.iconUrl,
+        excludeSportId: params.id,
+        reason: "replaced",
       });
     }
 
@@ -99,9 +108,7 @@ export async function action({ request, params }: Route.ActionArgs) {
   } catch (error) {
     logger.error("Error updating sport:", error);
     if (iconUrl && iconUrl !== existingSport?.iconUrl) {
-      deleteCloudinaryImageByUrl(iconUrl).catch((deleteError) => {
+      await deleteSportIconIfUnused({ iconUrl, excludeSportId: params.id, reason: "unused" });
-        logger.error("Failed to delete unused sport icon from Cloudinary:", deleteError);
-      });
     }
     const message = error instanceof Error ? error.message : String(error);
     const isUniqueViolation = message.includes("unique") || message.includes("duplicate");
@@ -114,7 +121,7 @@ export async function action({ request, params }: Route.ActionArgs) {
 }
 
 export default function EditSport({ loaderData, actionData }: Route.ComponentProps) {
-  const { sport } = loaderData;
+  const { sport, existingIconOptions } = loaderData;
 
   return (
     <div className="p-8">
@@ -222,6 +229,7 @@ export default function EditSport({ loaderData, actionData }: Route.ComponentPro
                   uploadUrl="/api/upload-sport-icon"
                   initialIconUrl={sport.iconUrl}
                   sportName={sport.name}
+                  existingIconOptions={existingIconOptions}
                 />
               </div>
 

app/routes/admin.sports.new.tsx

@@ -2,8 +2,8 @@ import { Form, Link, redirect } from "react-router";
 import type { Route } from "./+types/admin.sports.new";
 
 import { logger } from "~/lib/logger";
-import { createSport } from "~/models/sport";
+import { createSport, findAllSports } from "~/models/sport";
-import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server";
+import { deleteSportIconIfUnused } from "~/lib/sport-icon-cleanup.server";
 import { parseSportIconUrlInput } from "~/lib/sport-icon-url";
 import { Button } from "~/components/ui/button";
 import { Input } from "~/components/ui/input";
@@ -29,6 +29,15 @@ export function meta(): Route.MetaDescriptors {
   return [{ title: "New Sport - Brackt Admin" }];
 }
 
+export async function loader() {
+  const sports = await findAllSports();
+  return {
+    existingIconOptions: sports
+      .filter((sport) => Boolean(sport.iconUrl))
+      .map((sport) => ({ id: sport.id, name: sport.name, iconUrl: sport.iconUrl as string })),
+  };
+}
+
 export async function action({ request }: Route.ActionArgs) {
   const formData = await request.formData();
   const name = formData.get("name");
@@ -73,15 +82,13 @@ export async function action({ request }: Route.ActionArgs) {
   } catch (error) {
     logger.error("Error creating sport:", error);
     if (iconUrl) {
-      deleteCloudinaryImageByUrl(iconUrl).catch((deleteError) => {
+      await deleteSportIconIfUnused({ iconUrl, reason: "unused" });
-        logger.error("Failed to delete unused sport icon from Cloudinary:", deleteError);
-      });
     }
     return { error: "Failed to create sport. The slug might already exist." };
   }
 }
 
-export default function NewSport({ actionData }: Route.ComponentProps) {
+export default function NewSport({ loaderData, actionData }: Route.ComponentProps) {
   return (
     <div className="p-8">
       <div className="max-w-2xl">
@@ -155,7 +162,11 @@ export default function NewSport({ actionData }: Route.ComponentProps) {
 
               <div className="space-y-2">
                 <Label>Sport Icon (Optional)</Label>
-                <SportIconUploader uploadUrl="/api/upload-sport-icon" sportName="New sport" />
+                <SportIconUploader
+                  uploadUrl="/api/upload-sport-icon"
+                  sportName="New sport"
+                  existingIconOptions={loaderData.existingIconOptions}
+                />
               </div>
 
               {actionData?.error && (