From 2b8887910bd60da771a3d550eac82ade719ebe2b Mon Sep 17 00:00:00 2001 From: Matthew Robert Wesney <157447210+dovvnloading@users.noreply.github.com> Date: Mon, 7 Sep 2026 10:51:23 -0400 Subject: [PATCH] fix(frontend): keep the attachments that uploaded when one in the batch fails Attaching several files at once and having one fail discarded every file that had already uploaded. `addAttachments` accumulated into a local `staged` array and merged it into the composer draft only after the loop finished. Any throw inside the loop -- an oversized file, the combined-size ceiling, or a staging request failing -- jumped straight to the handler, and `staged` went out of scope unused. Those files were not merely missing from the UI. Each had already been uploaded and had a real backend artifact holding retention, so they were orphaned until it expired, and the user had to re-add the rest by hand with no indication which ones had made it. The merge is now a small helper called on both paths, so a partial batch keeps what genuinely uploaded, and the message says as much rather than reporting a bare failure. The same function also dropped any files beyond the remaining slots in silence: `files.slice(0, remaining)` with a message only in the fully-full case. Choosing nine files with eight slots attached eight and looked exactly like attaching nine. It now says how many were taken. The hardcoded "eight" in the neighbouring message became MAX_CHAT_ATTACHMENTS while I was there, since the new message reads from the same constant. Co-Authored-By: Claude Opus 5 --- frontend/src/features/chat/ChatPage.test.tsx | 60 ++++++++++++++++++++ frontend/src/features/chat/ChatPage.tsx | 56 +++++++++++------- 2 files changed, 97 insertions(+), 19 deletions(-) diff --git a/frontend/src/features/chat/ChatPage.test.tsx b/frontend/src/features/chat/ChatPage.test.tsx index c4cb43c..63ee285 100644 --- a/frontend/src/features/chat/ChatPage.test.tsx +++ b/frontend/src/features/chat/ChatPage.test.tsx @@ -1155,6 +1155,66 @@ describe("ChatPage composer integration", () => { expect(screen.getByRole("button", { name: "Remove next.md" })).toBeInTheDocument(); }); + it("keeps the files that uploaded when a later one in the batch fails", async () => { + // Each staged file is already uploaded and already holding backend + // retention. Committing the batch only after the whole loop meant one bad + // file discarded every good one before it, orphaning those artifacts and + // making the user re-add the rest by hand. + const user = userEvent.setup(); + const uploaded: ChatAttachment = { + attachment_id: "doc-good", + filename: "good.md", + mime_type: "text/markdown", + size: 4, + sha256: "a".repeat(64), + kind: "document", + expires_at: "2099-01-01T00:00:00Z", + }; + const api = chatApi({ + stageChatAttachment: vi.fn() + .mockResolvedValueOnce(uploaded) + .mockRejectedValueOnce(new Error("the second file could not be staged")), + }); + renderChat(api); + + const attachmentInput = await screen.findByLabelText("Attach images or documents"); + await user.upload(attachmentInput, [ + new File(["good"], "good.md", { type: "text/markdown" }), + new File(["bad"], "bad.md", { type: "text/markdown" }), + ]); + + await waitFor(() => expect(api.stageChatAttachment).toHaveBeenCalledTimes(2)); + expect(await screen.findByRole("button", { name: "Remove good.md" })).toBeInTheDocument(); + expect(await screen.findByRole("alert")).toHaveTextContent("Files attached before it were kept."); + }); + + it("says so when more files are chosen than a message can carry", async () => { + // Silently dropping the overflow looked exactly like attaching it. + const user = userEvent.setup(); + const api = chatApi({ + stageChatAttachment: vi.fn(async (request: { filename: string }) => ({ + attachment_id: `id-${request.filename}`, + filename: request.filename, + mime_type: "text/markdown", + size: 4, + sha256: "b".repeat(64), + kind: "document" as const, + expires_at: "2099-01-01T00:00:00Z", + })), + }); + renderChat(api); + + const attachmentInput = await screen.findByLabelText("Attach images or documents"); + await user.upload( + attachmentInput, + Array.from({ length: 9 }, (_unused, index) => + new File([`f${index}`], `file-${index}.md`, { type: "text/markdown" })), + ); + + await waitFor(() => expect(api.stageChatAttachment).toHaveBeenCalledTimes(8)); + expect(await screen.findByRole("alert")).toHaveTextContent("Only 8 of 9 files were attached"); + }); + it("explains the image capability mismatch before a generation request is made", async () => { const user = userEvent.setup(); const attachment: ChatAttachment = { diff --git a/frontend/src/features/chat/ChatPage.tsx b/frontend/src/features/chat/ChatPage.tsx index b7c6fdc..fa01ddd 100644 --- a/frontend/src/features/chat/ChatPage.tsx +++ b/frontend/src/features/chat/ChatPage.tsx @@ -637,12 +637,36 @@ export function ChatPage({ attachmentDraftTargetsRef.current.add(target); setAttachmentsBusy(true); setAttachmentError(null); + // Each staged file is already uploaded and already holding backend + // retention, so it belongs in the composer whether or not a later file in + // the same batch fails. Committing only after the whole loop meant one bad + // file discarded every good one before it -- leaving those artifacts + // orphaned on the backend and making the user re-add the rest by hand. + const staged: ChatAttachment[] = []; + const commitStaged = () => { + if (!staged.length) return; + // The generation request and attachment staging can finish in either + // order. Merge into the latest scoped draft instead of the render-time + // `attachments` snapshot, which may contain files that were submitted + // and cleared while these new files were still uploading. + const currentAttachments = attachmentDraftsRef.current[target.scope] + ?? readComposerAttachments(target.threadId); + const currentAttachmentIds = new Set(currentAttachments.map((attachment) => attachment.attachment_id)); + const next = [ + ...currentAttachments, + ...staged.filter((attachment) => !currentAttachmentIds.has(attachment.attachment_id)), + ]; + const nextAttachments = { ...attachmentDraftsRef.current, [target.scope]: next }; + attachmentDraftsRef.current = nextAttachments; + setAttachmentDrafts(nextAttachments); + writeComposerAttachments(target.threadId, next); + }; try { const remaining = Math.max(0, MAX_CHAT_ATTACHMENTS - attachments.length); - if (!remaining) throw new Error("A message can include at most eight attachments."); + if (!remaining) throw new Error(`A message can include at most ${MAX_CHAT_ATTACHMENTS} attachments.`); let totalBytes = attachments.reduce((total, attachment) => total + attachment.size, 0); - const staged: ChatAttachment[] = []; - for (const file of files.slice(0, remaining)) { + const accepted = files.slice(0, remaining); + for (const file of accepted) { if (!file.size || file.size > MAX_CHAT_ATTACHMENT_BYTES) { throw new Error(`${file.name} is empty or larger than 10 MB.`); } @@ -658,23 +682,17 @@ export function ChatPage({ staged.push(attachment); totalBytes += attachment.size; } - // The generation request and attachment staging can finish in either - // order. Merge into the latest scoped draft instead of the render-time - // `attachments` snapshot, which may contain files that were submitted - // and cleared while these new files were still uploading. - const currentAttachments = attachmentDraftsRef.current[target.scope] - ?? readComposerAttachments(target.threadId); - const currentAttachmentIds = new Set(currentAttachments.map((attachment) => attachment.attachment_id)); - const next = [ - ...currentAttachments, - ...staged.filter((attachment) => !currentAttachmentIds.has(attachment.attachment_id)), - ]; - const nextAttachments = { ...attachmentDraftsRef.current, [target.scope]: next }; - attachmentDraftsRef.current = nextAttachments; - setAttachmentDrafts(nextAttachments); - writeComposerAttachments(target.threadId, next); + commitStaged(); + if (accepted.length < files.length) { + // Dropping the overflow silently looked exactly like attaching it. + setAttachmentError( + `Only ${accepted.length} of ${files.length} files were attached; a message can include at most ${MAX_CHAT_ATTACHMENTS}.`, + ); + } } catch (error) { - setAttachmentError(error instanceof ApiError ? error.detail : error instanceof Error ? error.message : "The attachment could not be uploaded."); + commitStaged(); + const detail = error instanceof ApiError ? error.detail : error instanceof Error ? error.message : "The attachment could not be uploaded."; + setAttachmentError(staged.length ? `${detail} Files attached before it were kept.` : detail); } finally { attachmentDraftTargetsRef.current.delete(target); setAttachmentsBusy(false);