From d8025b172a4eb7d7a028ad1f446bcf88a85d010e Mon Sep 17 00:00:00 2001 From: Hridayesh Date: Thu, 30 Jul 2026 14:23:06 +0900 Subject: [PATCH 1/2] fix(admin): report failed media uploads as failures 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/media-upload-failure-reporting.md | 5 + packages/admin/src/router.tsx | 9 +- .../admin/tests/media-upload-failure.test.tsx | 148 ++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 .changeset/media-upload-failure-reporting.md create mode 100644 packages/admin/tests/media-upload-failure.test.tsx diff --git a/.changeset/media-upload-failure-reporting.md b/.changeset/media-upload-failure-reporting.md new file mode 100644 index 000000000..e611864b5 --- /dev/null +++ b/.changeset/media-upload-failure-reporting.md @@ -0,0 +1,5 @@ +--- +"@emdash-cms/admin": patch +--- + +Fixes the Media Library reporting a failed upload as a success. A rejected upload now shows an error naming the reason, instead of a green "File uploaded" message with nothing added to the list. diff --git a/packages/admin/src/router.tsx b/packages/admin/src/router.tsx index c39a32277..3592c8da3 100644 --- a/packages/admin/src/router.tsx +++ b/packages/admin/src/router.tsx @@ -1341,7 +1341,14 @@ function MediaPage() { isLoading={isLoading || isFetchingNextPage} hasMore={!!hasNextPage} onLoadMore={() => void fetchNextPage()} - onUpload={(file) => uploadMutation.mutate(file)} + // 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. + onUpload={async (file) => { + await uploadMutation.mutateAsync(file); + }} onLocalSearchChange={setSearch} onLocalMimeFilterChange={setMimeFilter} /> diff --git a/packages/admin/tests/media-upload-failure.test.tsx b/packages/admin/tests/media-upload-failure.test.tsx new file mode 100644 index 000000000..84a80d418 --- /dev/null +++ b/packages/admin/tests/media-upload-failure.test.tsx @@ -0,0 +1,148 @@ +import { Toasty } from "@cloudflare/kumo"; +/** + * 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 { i18n } from "@lingui/core"; +import { I18nProvider } from "@lingui/react"; +import { QueryClientProvider } from "@tanstack/react-query"; +import { RouterProvider } from "@tanstack/react-router"; +import * as React from "react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +import type { AdminManifest } from "../src/lib/api"; +import { createAdminRouter } from "../src/router"; +import { render } from "./utils/render.tsx"; +import { createMockFetch, createTestQueryClient } from "./utils/test-helpers"; + +const uploadMedia = vi.fn(); + +vi.mock("../src/lib/api", async () => { + const actual = await vi.importActual("../src/lib/api"); + return { + ...actual, + fetchMediaList: vi.fn().mockResolvedValue({ items: [], nextCursor: undefined }), + uploadMedia: (file: File) => uploadMedia(file) as Promise, + }; +}); + +/** Capture the props MediaPage hands to MediaLibrary. */ +let capturedOnUpload: ((file: File) => Promise | void) | undefined; + +vi.mock("../src/components/MediaLibrary", () => ({ + MediaLibrary: (props: { onUpload?: (file: File) => Promise | void }) => { + capturedOnUpload = props.onUpload; + return
; + }, +})); + +vi.mock("../src/components/Shell", () => ({ + Shell: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + +function buildRouter() { + const queryClient = createTestQueryClient(); + const router = createAdminRouter(queryClient); + if (!i18n.locale) { + i18n.loadAndActivate({ locale: "en", messages: {} }); + } + function TestApp() { + return ( + + + + + + + + ); + } + return { router, TestApp }; +} + +async function renderMediaPage() { + capturedOnUpload = undefined; + const { router, TestApp } = buildRouter(); + await router.navigate({ to: "/media" }); + const screen = await render(); + await expect.element(screen.getByTestId("media-library")).toBeInTheDocument(); + return capturedOnUpload; +} + +const MANIFEST: AdminManifest = { + version: "1.0.0", + hash: "abc123", + authMode: "passkey", + collections: {}, + plugins: {}, + taxonomies: [], + i18n: { defaultLocale: "en", locales: ["en"] }, +}; + +describe("MediaPage upload failure reporting", () => { + let mockFetch: ReturnType; + + beforeEach(() => { + uploadMedia.mockReset(); + mockFetch = createMockFetch(); + mockFetch + .on("GET", "/_emdash/api/manifest", { data: MANIFEST }) + .on("GET", "/_emdash/api/auth/me", { data: { id: "user_01", role: 60 } }); + }); + + afterEach(() => { + mockFetch.restore(); + }); + + it("rejects when the upload fails, so MediaLibrary can show the error", async () => { + uploadMedia.mockRejectedValue(new Error("File type not allowed")); + + const onUpload = await renderMediaPage(); + expect(onUpload).toBeDefined(); + + // mutate() would resolve to undefined here and the failure would be + // silently scored as a success. + await expect( + Promise.resolve(onUpload?.(new File(["x"], "notes.txt", { type: "text/plain" }))), + ).rejects.toThrow("File type not allowed"); + }); + + it("resolves when the upload succeeds", async () => { + uploadMedia.mockResolvedValue({ + id: "m1", + filename: "photo.png", + mimeType: "image/png", + url: "/_emdash/api/media/file/m1.png", + size: 10, + createdAt: "2026-01-01", + }); + + const onUpload = await renderMediaPage(); + expect(onUpload).toBeDefined(); + + // Companion check on the happy path -- the regression guard is the test + // above. The handler resolves to void, so the assertion is only that it + // settles without rejecting and that the upload was actually attempted. + await expect( + Promise.resolve(onUpload?.(new File(["x"], "photo.png", { type: "image/png" }))), + ).resolves.toBeUndefined(); + expect(uploadMedia).toHaveBeenCalledOnce(); + }); +}); From e0bc43da23e2abfedc74afaef542b8ab95167723 Mon Sep 17 00:00:00 2001 From: Hridayesh Date: Thu, 30 Jul 2026 14:39:14 +0900 Subject: [PATCH 2/2] docs: drop review narrative from the media upload comments 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. --- packages/admin/src/router.tsx | 5 ---- .../admin/tests/media-upload-failure.test.tsx | 26 ------------------- 2 files changed, 31 deletions(-) diff --git a/packages/admin/src/router.tsx b/packages/admin/src/router.tsx index 3592c8da3..dba723fc9 100644 --- a/packages/admin/src/router.tsx +++ b/packages/admin/src/router.tsx @@ -1341,11 +1341,6 @@ function MediaPage() { isLoading={isLoading || isFetchingNextPage} hasMore={!!hasNextPage} onLoadMore={() => void fetchNextPage()} - // 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. onUpload={async (file) => { await uploadMutation.mutateAsync(file); }} diff --git a/packages/admin/tests/media-upload-failure.test.tsx b/packages/admin/tests/media-upload-failure.test.tsx index 84a80d418..a4f4cd737 100644 --- a/packages/admin/tests/media-upload-failure.test.tsx +++ b/packages/admin/tests/media-upload-failure.test.tsx @@ -1,25 +1,4 @@ import { Toasty } from "@cloudflare/kumo"; -/** - * 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 { i18n } from "@lingui/core"; import { I18nProvider } from "@lingui/react"; import { QueryClientProvider } from "@tanstack/react-query"; @@ -117,8 +96,6 @@ describe("MediaPage upload failure reporting", () => { const onUpload = await renderMediaPage(); expect(onUpload).toBeDefined(); - // mutate() would resolve to undefined here and the failure would be - // silently scored as a success. await expect( Promise.resolve(onUpload?.(new File(["x"], "notes.txt", { type: "text/plain" }))), ).rejects.toThrow("File type not allowed"); @@ -137,9 +114,6 @@ describe("MediaPage upload failure reporting", () => { const onUpload = await renderMediaPage(); expect(onUpload).toBeDefined(); - // Companion check on the happy path -- the regression guard is the test - // above. The handler resolves to void, so the assertion is only that it - // settles without rejecting and that the upload was actually attempted. await expect( Promise.resolve(onUpload?.(new File(["x"], "photo.png", { type: "image/png" }))), ).resolves.toBeUndefined();