From 2d0568a14f0c57a7a8b6c5cc77ed4c13587f63de Mon Sep 17 00:00:00 2001 From: Jholly Date: Fri, 28 Aug 2026 10:40:29 +0800 Subject: [PATCH] fix(frontend): format structured upload error details (#5071) * fix(frontend): format structured upload error details * fix(frontend): preserve generic message error details --- frontend/src/core/uploads/api.ts | 79 +++++++++++++- frontend/tests/e2e/chat.spec.ts | 39 +++++++ frontend/tests/unit/core/uploads/api.test.ts | 108 ++++++++++++++++++- 3 files changed, 223 insertions(+), 3 deletions(-) diff --git a/frontend/src/core/uploads/api.ts b/frontend/src/core/uploads/api.ts index a12688685..9115f98fc 100644 --- a/frontend/src/core/uploads/api.ts +++ b/frontend/src/core/uploads/api.ts @@ -37,12 +37,87 @@ export interface UploadLimits { max_total_size: number; } +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function formatValidationIssue(issue: unknown): string | null { + if ( + !isRecord(issue) || + typeof issue.msg !== "string" || + !Array.isArray(issue.loc) + ) { + return null; + } + + if (issue.msg.trim().length === 0) { + return null; + } + + const location = issue.loc + .filter( + (part): part is string | number => + typeof part === "string" || typeof part === "number", + ) + .map(String) + .filter((part) => part.length > 0) + .join("."); + + return location.length > 0 ? `${location}: ${issue.msg}` : issue.msg; +} + +function serializeStructuredDetail( + detail: Record | unknown[], +): string | null { + if ( + (Array.isArray(detail) && detail.length === 0) || + (!Array.isArray(detail) && Object.keys(detail).length === 0) + ) { + return null; + } + + try { + return JSON.stringify(detail); + } catch { + return null; + } +} + +function formatErrorDetail(detail: unknown): string | null { + if (typeof detail === "string") { + return detail.trim().length > 0 ? detail : null; + } + + if (Array.isArray(detail)) { + if (detail.length === 0) { + return null; + } + + const validationIssues = detail.map(formatValidationIssue); + if (validationIssues.every((issue) => issue !== null)) { + return validationIssues.join("; "); + } + + return serializeStructuredDetail(detail); + } + + if (isRecord(detail)) { + return formatValidationIssue(detail) ?? serializeStructuredDetail(detail); + } + + return null; +} + async function readErrorDetail( response: Response, fallback: string, ): Promise { - const error = await response.json().catch(() => ({ detail: fallback })); - return error.detail ?? fallback; + const error = (await response.json().catch(() => null)) as unknown; + if (!isRecord(error)) { + return fallback; + } + + return formatErrorDetail(error.detail) ?? fallback; } /** diff --git a/frontend/tests/e2e/chat.spec.ts b/frontend/tests/e2e/chat.spec.ts index 859e48957..952d0e97f 100644 --- a/frontend/tests/e2e/chat.spec.ts +++ b/frontend/tests/e2e/chat.spec.ts @@ -1060,6 +1060,45 @@ test.describe("Chat workspace", () => { await expect(page.getByRole("tooltip")).toContainText("100 MiB"); }); + test("shows structured upload errors as readable messages", async ({ + page, + }) => { + await page.route("**/api/threads/*/uploads", (route) => + route.fulfill({ + status: 422, + contentType: "application/json", + body: JSON.stringify({ + detail: [ + { + type: "missing", + loc: ["body", "files"], + msg: "Field required", + input: null, + }, + ], + }), + }), + ); + + await page.goto("/workspace/chats/new"); + const textarea = page.getByPlaceholder(/how can i assist you/i); + await expect(textarea).toBeVisible({ timeout: 15_000 }); + + await page.getByLabel("Upload files").setInputFiles({ + name: "report.txt", + mimeType: "text/plain", + buffer: Buffer.from("report"), + }); + await textarea.fill("Summarize this report"); + await textarea.press("Enter"); + + const errorToast = page + .locator("[data-sonner-toast]") + .filter({ hasText: "body.files: Field required" }); + await expect(errorToast).toBeVisible(); + await expect(errorToast).not.toContainText("[object Object]"); + }); + test("rejects an oversized attachment before upload", async ({ page }) => { let uploadCalled = false; await page.route("**/api/threads/*/uploads", (route) => { diff --git a/frontend/tests/unit/core/uploads/api.test.ts b/frontend/tests/unit/core/uploads/api.test.ts index 3cdb83a4e..72ed3fdbb 100644 --- a/frontend/tests/unit/core/uploads/api.test.ts +++ b/frontend/tests/unit/core/uploads/api.test.ts @@ -9,7 +9,7 @@ rs.mock("@/core/config", () => ({ })); import { fetch as fetcher } from "@/core/api/fetcher"; -import { deleteUploadedFile } from "@/core/uploads/api"; +import { deleteUploadedFile, uploadFiles } from "@/core/uploads/api"; const mockedFetch = rs.mocked(fetcher); @@ -21,11 +21,117 @@ function jsonResponse(status: number, body: unknown): Response { }); } +async function uploadError(response: Response): Promise { + mockedFetch.mockResolvedValueOnce(response); + + let thrown: unknown; + try { + await uploadFiles("thread-1", []); + } catch (error) { + thrown = error; + } + + expect(thrown).toBeInstanceOf(Error); + return thrown as Error; +} + beforeEach(() => { mockedFetch.mockReset(); }); describe("uploads api", () => { + test("preserves string error details", async () => { + const error = await uploadError( + jsonResponse(413, { detail: "File exceeds the upload limit" }), + ); + + expect(error.message).toBe("File exceeds the upload limit"); + }); + + test("formats FastAPI validation error details", async () => { + const error = await uploadError( + jsonResponse(422, { + detail: [ + { + type: "missing", + loc: ["body", "files"], + msg: "Field required", + input: null, + }, + { + type: "value_error", + loc: ["body", "files", 0], + msg: "File is empty", + input: "", + }, + ], + }), + ); + + expect(error.message).toBe( + "body.files: Field required; body.files.0: File is empty", + ); + expect(error.message).not.toContain("[object Object]"); + }); + + test("serializes object error details", async () => { + const error = await uploadError( + jsonResponse(400, { + detail: { code: "invalid_archive", reason: "Archive is corrupt" }, + }), + ); + + expect(error.message).toBe( + '{"code":"invalid_archive","reason":"Archive is corrupt"}', + ); + expect(error.message).not.toContain("[object Object]"); + }); + + test.each([ + [ + "object", + { msg: "Archive rejected", code: "invalid_archive", retryable: false }, + '{"msg":"Archive rejected","code":"invalid_archive","retryable":false}', + ], + [ + "array", + [{ msg: "Archive rejected", code: "invalid_archive", retryable: false }], + '[{"msg":"Archive rejected","code":"invalid_archive","retryable":false}]', + ], + ])( + "serializes a generic %s detail containing msg without a validation loc", + async (_label, detail, expected) => { + const error = await uploadError(jsonResponse(400, { detail })); + + expect(error.message).toBe(expected); + expect(error.message).not.toContain("[object Object]"); + }, + ); + + test.each([ + ["missing", {}], + ["null", { detail: null }], + ["blank string", { detail: " " }], + ["empty array", { detail: [] }], + ["empty object", { detail: {} }], + ["unexpected scalar", { detail: 42 }], + ])("uses the upload fallback for %s detail", async (_label, body) => { + const error = await uploadError(jsonResponse(400, body)); + + expect(error.message).toBe("Upload failed"); + }); + + test("uses the upload fallback for non-JSON responses", async () => { + const error = await uploadError( + new Response("Bad Gateway", { + status: 502, + headers: { "Content-Type": "text/plain" }, + }), + ); + + expect(error.message).toBe("Upload failed"); + }); + test("encodes uploaded filenames in delete request paths", async () => { mockedFetch.mockResolvedValueOnce( jsonResponse(200, {