Skip to content

fix(uploads): detect upload success via response.ok so the editor stops failing every image #1339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
NiallJoeMaher merged 1 commit into from
Jun 18, 2026
+61 −7
Merged

fix(uploads): detect upload success via response.ok so the editor stops failing every image #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/(admin)/admin/sources/_client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions app/(app)/jobs/create/_client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
8 changes: 6 additions & 2 deletions app/(app)/settings/_client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down
45 changes: 45 additions & 0 deletions utils/s3helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
});
Comment on lines +8 to +10

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Vitest v4.x, does vi.restoreAllMocks()revert globals stubbed withvi.stubGlobal()? If not, is vi.unstubAllGlobals() the intended cleanup API?

💡 Result:

In Vitest v4.x, vi.restoreAllMocks does not revert global variables stubbed with vi.stubGlobal [1][2]. vi.restoreAllMocks is specifically designed to restore the original implementations of spies created via vi.spyOn [1][2]. vi.unstubAllGlobals is the correct and intended API for cleaning up stubs created with vi.stubGlobal [1][3]. When you call vi.stubGlobal, Vitest tracks the original value (or state) of the global variable so that vi.unstubAllGlobals can restore it to its previous state [1][3]. Alternatively, you can enable the unstubGlobals configuration option in your Vitest config file to have Vitest automatically call vi.unstubAllGlobals after each test [4][5].

Citations:

🏁 Script executed:

fd "s3helpers.test.ts" -x wc -l {}

Repository: codu-code/codu

Length of output: 87

🏁 Script executed:

fd "s3helpers.test.ts" -x cat -n {}

Repository: codu-code/codu

Length of output: 1844

Unstub global fetch in teardown to prevent test-suite leakage.

The afterEach block at lines 8-10 calls vi.restoreAllMocks(), but this does not clean up globals stubbed with vi.stubGlobal("fetch", ...) at line 16. Each test will leave the stubbed fetch in place, potentially affecting subsequent tests. Add vi.unstubAllGlobals() to the teardown:

Suggested patch 
  afterEach(() => {
    vi.restoreAllMocks();
+   vi.unstubAllGlobals();
  });
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/s3helpers.test.ts` around lines 8 - 10, In the afterEach block in
utils/s3helpers.test.ts, add a call to vi.unstubAllGlobals() after or alongside
the existing vi.restoreAllMocks() call to ensure that any globals stubbed with
vi.stubGlobal (such as the fetch stub) are properly cleaned up after each test.
This will prevent stubbed globals from leaking into subsequent tests.


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);
});
});
4 changes: 3 additions & 1 deletion utils/s3helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
Loading