fix(admin): report failed media uploads as failures - #2283
Conversation
MediaPage passed `uploadMutation.mutate` as MediaLibrary's `onUpload`. MediaLibrary awaits that call and counts rejections to choose between the success and the error banner, but `mutate()` is fire-and-forget: it returns undefined and never rejects. Every upload was therefore scored a success, so a rejected upload rendered the green "File uploaded" banner while nothing appeared in the list and the API's reason was discarded -- leaving no way to tell a broken configuration from a working one. Await `mutateAsync` instead so the rejection reaches the caller.
🦋 Changeset detectedLatest commit: e0bc43d The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is the right change for the reported bug. MediaLibrary already awaits onUpload and counts rejections to choose between the success and error banner, but mutate() is fire-and-forget, so the rejection from uploadMedia never reached it. Switching the MediaPage wiring to mutateAsync propagates the error with a minimal, well-scoped source change.
I reviewed statically — no tests, build, or install were run. Checks made:
- Confirmed
MediaLibrary.handleFileSelectawaitsonUploadand incrementsfailedonly on rejection. - Verified the other
on*mutation handlers inrouter.tsx(e.g.,onDelete,onSave) pass.mutateto components that drive their own pending/error UI, so this fire-and-forget pattern isn't a systemic issue elsewhere. - Confirmed the new test covers both the rejection path and the happy path, and fails if the handler is reverted to
mutate. - No new admin UI strings, SQL, server routes, or logged-out queries are introduced. The changeset is present and correctly targets
@emdash-cms/admin.
The only real issue is a comment-discipline violation in the source: the five-line inline comment above onUpload is PR-description prose that restates the bug and justifies the implementation choice. AGENTS.md says comments should not summarize the change or justify decisions; that context belongs in the changeset and commit message. The code and type signature already make the contract obvious.
I also noticed the new test file carries a similarly verbose opening docblock and a couple of inline justifying comments; these should be trimmed for the same reason, but they don't affect behavior.
No blocking correctness, security, or performance issues.
| // mutateAsync, not mutate: MediaLibrary awaits this call and counts | ||
| // rejections to decide between the success and error banner. mutate() | ||
| // never rejects, so a failed upload was scored as a success -- the | ||
| // green "File uploaded" banner appeared and nothing was added to the | ||
| // list, with the API's reason discarded. |
There was a problem hiding this comment.
[needs fixing] This five-line comment is PR-description prose: it restates the bug, names the green banner, and justifies choosing mutateAsync over mutate. AGENTS.md prohibits comments that summarize the change or justify a decision — that context belongs in the changeset and commit message. The wrapper itself already makes the await/reject contract clear. Remove the comment block.
| // mutateAsync, not mutate: MediaLibrary awaits this call and counts | |
| // rejections to decide between the success and error banner. mutate() | |
| // never rejects, so a failed upload was scored as a success -- the | |
| // green "File uploaded" banner appeared and nothing was added to the | |
| // list, with the API's reason discarded. | |
| onLoadMore={() => void fetchNextPage()} | |
| onUpload={async (file) => { | |
| await uploadMutation.mutateAsync(file); | |
| }} |
There was a problem hiding this comment.
Fixed in e0bc43d — comment block removed, wrapper left as-is.
| /** | ||
| * MediaPage must surface a failed upload as a failure. | ||
| * | ||
| * `MediaLibrary.handleFileSelect` awaits the `onUpload` prop and decides between | ||
| * the success and error banner by counting rejections: | ||
| * | ||
| * try { await onUpload?.(file); uploaded++ } | ||
| * catch { failed++ } | ||
| * ... | ||
| * if (failed === 0) setUploadState({ status: "success", ... }) | ||
| * | ||
| * MediaPage used to pass `uploadMutation.mutate`, which is fire-and-forget — it | ||
| * returns `undefined` and never rejects. So `failed` stayed 0 no matter what the | ||
| * API said, and a rejected upload rendered the green "File uploaded" banner | ||
| * while nothing was added to the list, with the API's reason discarded. Passing | ||
| * `mutateAsync` propagates the real error to that catch. | ||
| * | ||
| * The assertion is on the prop contract rather than the rendered banner: the | ||
| * banner is MediaLibrary's behaviour (and is already correct), whereas the bug | ||
| * was purely in how MediaPage wired the handler. | ||
| */ |
There was a problem hiding this comment.
[suggestion] The opening docblock is a condensed bug narrative rather than a test-purpose comment. AGENTS.md discourages comments that read like PR descriptions. The test name and code already explain what is being verified. Trim or remove this block, and consider deleting the inline justifying comments on lines 120 and 140 as well.
| /** | |
| * MediaPage must surface a failed upload as a failure. | |
| * | |
| * `MediaLibrary.handleFileSelect` awaits the `onUpload` prop and decides between | |
| * the success and error banner by counting rejections: | |
| * | |
| * try { await onUpload?.(file); uploaded++ } | |
| * catch { failed++ } | |
| * ... | |
| * if (failed === 0) setUploadState({ status: "success", ... }) | |
| * | |
| * MediaPage used to pass `uploadMutation.mutate`, which is fire-and-forget — it | |
| * returns `undefined` and never rejects. So `failed` stayed 0 no matter what the | |
| * API said, and a rejected upload rendered the green "File uploaded" banner | |
| * while nothing was added to the list, with the API's reason discarded. Passing | |
| * `mutateAsync` propagates the real error to that catch. | |
| * | |
| * The assertion is on the prop contract rather than the rendered banner: the | |
| * banner is MediaLibrary's behaviour (and is already correct), whereas the bug | |
| * was purely in how MediaPage wired the handler. | |
| */ | |
| import { Toasty } from "@cloudflare/kumo"; | |
| import { i18n } from "@lingui/core"; |
There was a problem hiding this comment.
Fixed in e0bc43d — docblock removed, along with the two inline justifying comments you flagged.
AGENTS.md scopes comments to a future reader of the code: not PR descriptions, not justification for a decision, not narrative about what was tried. The blocks added with the fix were all three. The await/reject contract is legible from the wrapper itself and the rest lives in the commit message and changeset.
github-actions
Bot
added
review/needs-rereview
|
Closing this — #2273 landed while it was in review and already carries the fix.
The only thing this PR still adds is Thanks for the review passes; the comment-discipline notes were fair and I applied them in e0bc43d. Happy to put the test up as a standalone PR if you'd want it. |
1 participant
What does this PR do?
MediaPagepasseduploadMutation.mutateasMediaLibrary'sonUpload.MediaLibraryawaits that call and counts rejections to decide between the success and the error banner — butmutate()is fire-and-forget: it returnsundefinedand never rejects. Every upload was therefore scored a success, so a rejected upload rendered the green "File uploaded" banner while nothing appeared in the list, and the reason the API gave (File type not allowed,File exceeds maximum size of …) was discarded.The fix is to await
mutateAsyncso the rejection reaches the caller that is already written to handle it. No change toMediaLibraryitself.This is worth more than the one-line diff suggests: with the banner lying and auto-clearing after 3s, a misconfigured install is indistinguishable from a working one. It sent our QA down a multi-day hunt for a broken upload pipeline that was never broken.
Closes #2282
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
packages/admin/tests/media-upload-failure.test.tsxasserts thatonUploadrejects when the upload fails. It detects a revert — with the fix backed out and everything else unchanged:With the fix applied:
Full suites on this branch's base (
668184f):Two notes on the shared gates, both reproduced on a clean checkout of
main@668184fwith no changes applied, i.e. pre-existing and unrelated to this PR:pnpm typecheckfails inpackages/core—src/plugins/context.ts(1212,24): error TS2345: Argument of type '"cache:purge"' is not assignable to parameter of type 'PluginCapability'.pnpm lintreports 1 warning (and--deny-warningsturns it into a failure) —typescript(no-unnecessary-type-assertion)atpackages/cloudflare/src/sandbox/bridge.ts:1210.Happy to rebase once those are sorted if it makes CI easier to read.