From b9011a9d4e872b9de8f645a7320fa5d627ecf46b Mon Sep 17 00:00:00 2001
From: NiallJoeMaher <nialljoemaher@gmail.com>
Date: Wed, 17 Jun 2026 16:50:41 +0100
Subject: [PATCH] fix(uploads): detect upload success via response.ok so the
 editor stops failing every image
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

uploadFile returned `{ ...response, fileLocation }`, but a fetch Response
exposes ok/status/url as prototype getters, not own enumerable properties —
so the spread dropped them and callers read `undefined` for `ok`.

The article editor's `if (!uploadResult.ok) throw` then reported failure on
*every* image upload, even though the presigned PUT returned 200 and the file
landed in S3. (The recent bucket-name trim fixed signing; this was the
remaining reason uploads still showed an error.)

Return ok/status explicitly, and harden the other callers (profile photo, job
logo, admin source logo) to check `ok` instead of trusting a fileLocation that
is derived from the URL regardless of whether the PUT succeeded. Adds a
regression test for uploadFile.
---
 app/(admin)/admin/sources/_client.tsx |  7 +++--
 app/(app)/jobs/create/_client.tsx     |  4 +--
 app/(app)/settings/_client.tsx        |  8 +++--
 utils/s3helpers.test.ts               | 45 +++++++++++++++++++++++++++
 utils/s3helpers.ts                    |  4 ++-
 5 files changed, 61 insertions(+), 7 deletions(-)
 create mode 100644 utils/s3helpers.test.ts

diff --git a/app/(admin)/admin/sources/_client.tsx b/app/(admin)/admin/sources/_client.tsx
index abec554fd..63aff433a 100644
--- a/app/(admin)/admin/sources/_client.tsx
+++ b/app/(admin)/admin/sources/_client.tsx
@@ -242,8 +242,11 @@ const AdminSourcesPage = () => {
         },
         async onSuccess(signedUrl) {
           try {
-            const response = await uploadFile(signedUrl, file);
-            const { fileLocation } = response;
+            const { ok, fileLocation } = await uploadFile(signedUrl, file);
+            if (!ok) {
+              toast.error("Failed to upload logo, please try again.");
+              return;
+            }
             setEditingSource({
               ...editingSource,
               logoUrl: fileLocation,
diff --git a/app/(app)/jobs/create/_client.tsx b/app/(app)/jobs/create/_client.tsx
index 1bcb76c80..55a15ae97 100644
--- a/app/(app)/jobs/create/_client.tsx
+++ b/app/(app)/jobs/create/_client.tsx
@@ -127,8 +127,8 @@ export default function Content() {
           );
         }
 
-        const { fileLocation } = await uploadFile(signedUrl, file);
-        if (!fileLocation) {
+        const { ok, fileLocation } = await uploadFile(signedUrl, file);
+        if (!ok || !fileLocation) {
           setUploadStatus("error");
           return toast.error(
             "Something went wrong uploading the logo, please retry.",
diff --git a/app/(app)/settings/_client.tsx b/app/(app)/settings/_client.tsx
index 64532f9b0..79384606e 100644
--- a/app/(app)/settings/_client.tsx
+++ b/app/(app)/settings/_client.tsx
@@ -201,8 +201,12 @@ const Settings = ({ profile }: { profile: User }) => {
       return;
     }
 
-    const response = await uploadFile(signedUrl, file);
-    const { fileLocation } = response;
+    const { ok, fileLocation } = await uploadFile(signedUrl, file);
+    if (!ok) {
+      setProfilePhoto({ status: "error", url: "" });
+      return;
+    }
+
     await updateUserPhotoUrl({
       url: fileLocation,
     });
diff --git a/utils/s3helpers.test.ts b/utils/s3helpers.test.ts
new file mode 100644
index 000000000..dc55c3527
--- /dev/null
+++ b/utils/s3helpers.test.ts
@@ -0,0 +1,45 @@
+import { describe, it, expect, vi, afterEach } from "vitest";
+import { uploadFile } from "./s3helpers";
+
+// Regression guard: spreading a Response (`{ ...response }`) dropped its
+// prototype getters, so `result.ok` came back `undefined` and the editor
+// reported failure on every upload — even on a 200.
+describe("uploadFile", () => {
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  function mockPutResponse(status: number, url: string) {
+    const response = new Response(null, { status });
+    // Response.url is read-only via the constructor; pin it for fileLocation.
+    Object.defineProperty(response, "url", { value: url });
+    vi.stubGlobal("fetch", vi.fn().mockResolvedValue(response));
+  }
+
+  const file = new Blob(["x"], { type: "image/png" }) as unknown as File;
+
+  it("exposes ok=true on a successful PUT so callers can detect success", async () => {
+    mockPutResponse(
+      200,
+      "https://bucket.s3.amazonaws.com/uploads/u1/abc.png?sig=x",
+    );
+
+    const result = await uploadFile("https://signed-url", file);
+
+    expect(result.ok).toBe(true);
+    expect(result.fileLocation).toBe(
+      "https://bucket.s3.amazonaws.com/uploads/u1/abc.png",
+    );
+  });
+
+  it("exposes ok=false when S3 rejects the PUT", async () => {
+    mockPutResponse(
+      403,
+      "https://bucket.s3.amazonaws.com/uploads/u1/abc.png?sig=x",
+    );
+
+    const result = await uploadFile("https://signed-url", file);
+
+    expect(result.ok).toBe(false);
+  });
+});
diff --git a/utils/s3helpers.ts b/utils/s3helpers.ts
index e70b1717d..9e26a0294 100644
--- a/utils/s3helpers.ts
+++ b/utils/s3helpers.ts
@@ -27,5 +27,7 @@ export const uploadFile = async (signedUrl: string, file: File) => {
   });
 
   const fileLocation = response.url.split("?")[0];
-  return { ...response, fileLocation };
+  // Pull fields off explicitly — spreading a Response drops its prototype
+  // getters (ok/status), leaving callers to read `undefined` for `ok`.
+  return { ok: response.ok, status: response.status, fileLocation };
 };