fix(uploads): detect upload success via response.ok so the editor stops failing every image#1339
fix(uploads): detect upload success via response.ok so the editor stops failing every image#1339NiallJoeMaher wants to merge 1 commit into
Conversation
…ps failing every image
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough
ChangesuploadFile Contract Fix and Callsite Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@utils/s3helpers.test.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0be6b4e6-9423-483b-b13a-1ddb4eb46b7d
📒 Files selected for processing (5)
app/(admin)/admin/sources/_client.tsxapp/(app)/jobs/create/_client.tsxapp/(app)/settings/_client.tsxutils/s3helpers.test.tsutils/s3helpers.ts
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧩 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:
- 1: https://github.com/vitest-dev/vitest/blob/v4.1.4/docs/api/vi.md
- 2: https://vitest.dev/api/vi
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.7/docs/api/vi.md
- 4: https://vitest.dev/guide/mocking.html
- 5: https://vitest.dev/guide/mocking/globals
🏁 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.
1 participant
What
Image uploads in the article editor showed "Failed to upload image" on every attempt, even though the file actually reached S3.
uploadFilereturned{ ...response, fileLocation }. A fetchResponseexposesok,status, andurlas prototype getters, not own-enumerable properties, so the spread copied none of them — callers readundefinedforok.The editor's success check is
if (!uploadResult.ok) throw new Error("Failed to upload image"), so!undefinedthrew on every upload. The presignedPUTreturned200and the object landed in S3, but the UI reported failure and never inserted the image.This was the remaining cause after the recent bucket-name trim (which fixed signing).
How
uploadFilenow returns{ ok, status, fileLocation }explicitly instead of spreading theResponse.ok. Previously they trustedfileLocation, which is derived fromresponse.urland is truthy even on a failedPUT, so a failed upload could silently be treated as success.uploadFilecovering theok-on-success /ok-on-failure contract.Verification
result.ok→undefined) and passes with the fix.PUTreturns200, object loads as an<img>— only the client-sideokcheck was wrong.prettier,eslint, andtscclean on the changed files; unit tests pass.