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);