From a9c0082e9afbeb8589a9767aadc83260fc6433da Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:05:19 +0100 Subject: [PATCH] Send with a failed attachment instead of blocking A failed attachment made getSendReadiness return 'blocked', so Send was a no-op until the user found and removed the chip. On a phone the only hint was a small red line after the tap, and a new user lost their question that way. A failed file never recovers, so holding the send only costs the question. Readiness now ignores failed attachments: it waits only on uploading or processing files. On submit the composer drops every failed attachment and sends the text with the completed ones. The payloads already read completed attachments only, so no failed id reaches the request or the history. The hero-card path in Conversation also clears failed chips. The failed chip still shows before Send and can be removed by hand. Removes the send-blocked alert and its locale key. --- frontend/src/components/MessageInput.test.tsx | 139 ++++++++++++++++++ frontend/src/components/MessageInput.tsx | 36 +++-- .../message-input/armedSend.test.tsx | 32 ++-- .../src/components/message-input/armedSend.ts | 11 +- frontend/src/conversation/Conversation.tsx | 10 +- .../conversation/conversationSlice.test.ts | 18 +++ frontend/src/locale/en.json | 1 - 7 files changed, 203 insertions(+), 44 deletions(-) create mode 100644 frontend/src/components/MessageInput.test.tsx diff --git a/frontend/src/components/MessageInput.test.tsx b/frontend/src/components/MessageInput.test.tsx new file mode 100644 index 00000000..2d5e048a --- /dev/null +++ b/frontend/src/components/MessageInput.test.tsx @@ -0,0 +1,139 @@ +import { configureStore } from '@reduxjs/toolkit'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { Provider } from 'react-redux'; + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); +// The upload modal pulls in the whole ingest UI; the composer never opens it here. +vi.mock('../upload/Upload', () => ({ default: () => null })); + +import notificationsReducer from '../notifications/notificationsSlice'; +import { prefSlice } from '../preferences/preferenceSlice'; +import type { RootState } from '../store'; +import uploadReducer, { + addAttachment, + selectCompletedAttachments, + type Attachment, +} from '../upload/uploadSlice'; +import MessageInput from './MessageInput'; + +Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + +const makeStore = () => + configureStore({ + reducer: { + preference: prefSlice.reducer, + upload: uploadReducer, + notifications: notificationsReducer, + }, + }); + +type TestStore = ReturnType; + +const att = (over: Partial = {}): Attachment => ({ + id: 'ok-1', + fileName: 'notes.pdf', + progress: 100, + status: 'completed', + taskId: 't1', + ...over, +}); + +describe('MessageInput send with a failed attachment', () => { + let container: HTMLDivElement; + let root: Root; + let store: TestStore; + let onSubmit: ReturnType void>>; + + beforeEach(() => { + localStorage.clear(); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + store = makeStore(); + onSubmit = vi.fn<(text: string) => void>(); + }); + + afterEach(async () => { + await act(async () => root.unmount()); + container.remove(); + }); + + const render = async (loading = false) => { + await act(async () => { + root.render( + + + , + ); + }); + }; + + const textarea = () => + container.querySelector('#message-input')!; + + const type = async (text: string) => { + const el = textarea(); + await act(async () => { + const setter = Object.getOwnPropertyDescriptor( + HTMLTextAreaElement.prototype, + 'value', + )!.set!; + setter.call(el, text); + el.dispatchEvent(new Event('input', { bubbles: true })); + }); + }; + + const pressEnter = async () => { + await act(async () => { + textarea().dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }), + ); + }); + }; + + it('sends the question and drops the failed file', async () => { + store.dispatch(addAttachment(att())); + store.dispatch( + addAttachment( + att({ id: 'bad-1', fileName: 'broken.pdf', status: 'failed' }), + ), + ); + let idsAtSubmit: string[] = []; + onSubmit.mockImplementation(() => { + idsAtSubmit = selectCompletedAttachments( + store.getState() as unknown as RootState, + ).map((a) => a.id); + }); + await render(); + + await type('What does the contract say?'); + await pressEnter(); + + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit).toHaveBeenCalledWith('What does the contract say?'); + expect(idsAtSubmit).toEqual(['ok-1']); + expect(store.getState().upload.attachments.map((a) => a.id)).toEqual([ + 'ok-1', + ]); + expect(textarea().value).toBe(''); + }); + + it('shows no send-blocked alert after a failed send attempt', async () => { + store.dispatch(addAttachment(att({ id: 'bad-1', status: 'failed' }))); + await render(); + + await type('hello'); + await pressEnter(); + + expect(container.querySelector('[role="alert"]')).toBeNull(); + }); +}); diff --git a/frontend/src/components/MessageInput.tsx b/frontend/src/components/MessageInput.tsx index c9a90216..1a5327d6 100644 --- a/frontend/src/components/MessageInput.tsx +++ b/frontend/src/components/MessageInput.tsx @@ -1578,10 +1578,12 @@ export default function MessageInput({ }; // When ``allowSendWithoutText`` is set, an attachment-only submit is - // permitted as long as at least one attachment exists; a still-pending - // one arms the send instead of submitting (see handleSubmit). + // permitted as long as at least one attachment can still go out; a + // still-pending one arms the send instead of submitting (see handleSubmit). + // A failed one doesn't count: it is dropped at submit time. const hasSubmittableContent = - Boolean(value.trim()) || (allowSendWithoutText && attachments.length > 0); + Boolean(value.trim()) || + (allowSendWithoutText && attachments.some((a) => a.status !== 'failed')); const canSubmit = hasSubmittableContent && !loading && @@ -1589,6 +1591,17 @@ export default function MessageInput({ recordingState !== 'transcribing'; const submitNow = () => { + const failed = attachments.filter((a) => a.status === 'failed'); + const hasContent = + Boolean(value.trim()) || + (allowSendWithoutText && + attachments.some((a) => a.status === 'completed')); + // An attachment-only send whose files all failed has nothing left to + // send; keep the failed chips so the user can see why. + if (!hasContent) return; + // A failed file will never succeed, so it must never cost the user their + // question: drop it and send with whatever did upload. + failed.forEach((a) => dispatch(removeAttachment(a.id))); onSubmit(value); setValue(''); if (isTouch) { @@ -1621,9 +1634,10 @@ export default function MessageInput({ const handleSubmit = () => { if (!canSubmit) return; - // Attachments still uploading/parsing (or failed) must never be - // silently dropped from the payload: hold the send in the composer - // until every attachment resolves, then flush automatically. + // Attachments still uploading/parsing must never be silently dropped + // from the payload: hold the send in the composer until every + // attachment resolves, then flush automatically. Failed ones don't hold + // it; submitNow drops them. if (sendReadiness.state !== 'ready') { armSend(); return; @@ -1722,16 +1736,6 @@ export default function MessageInput({ )} - {sendArmed && sendReadiness.state === 'blocked' && ( -
- {t('conversation.attachments.sendBlockedByFailed', { - names: sendReadiness.failedNames.join(', '), - })} -
- )} {voiceError && (
{voiceError} diff --git a/frontend/src/components/message-input/armedSend.test.tsx b/frontend/src/components/message-input/armedSend.test.tsx index f1f602a3..519b5030 100644 --- a/frontend/src/components/message-input/armedSend.test.tsx +++ b/frontend/src/components/message-input/armedSend.test.tsx @@ -36,22 +36,28 @@ describe('getSendReadiness', () => { ).toEqual({ state: 'waiting', pendingCount: 2 }); }); - it('blocks on a failed attachment, listing its name', () => { + it('ignores a failed attachment: it is dropped at send time', () => { expect( getSendReadiness([ att(), att({ id: 'a2', status: 'failed', fileName: 'broken.pdf' }), ]), - ).toEqual({ state: 'blocked', failedNames: ['broken.pdf'] }); + ).toEqual({ state: 'ready' }); }); - it('failed takes precedence over pending', () => { + it('is ready when every attachment failed', () => { + expect(getSendReadiness([att({ status: 'failed' })])).toEqual({ + state: 'ready', + }); + }); + + it('still waits on a pending file next to a failed one', () => { expect( getSendReadiness([ - att({ status: 'processing' }), + att({ status: 'uploading', progress: 5 }), att({ id: 'a2', status: 'failed', fileName: 'broken.pdf' }), ]), - ).toEqual({ state: 'blocked', failedNames: ['broken.pdf'] }); + ).toEqual({ state: 'waiting', pendingCount: 1 }); }); }); @@ -120,21 +126,17 @@ describe('useArmedSend', () => { expect(onFlush).toHaveBeenCalledTimes(1); }); - it('holds the flush while a file is failed and resumes when it is removed', async () => { - await render([att({ status: 'processing' })]); + it('flushes once when the pending file fails instead of completing', async () => { + await render([att({ status: 'uploading', progress: 5 })]); await act(async () => api.current!.arm()); + expect(onFlush).not.toHaveBeenCalled(); await render([att({ status: 'failed', fileName: 'broken.pdf' })]); - expect(onFlush).not.toHaveBeenCalled(); - expect(api.current!.armed).toBe(true); - expect(api.current!.readiness).toEqual({ - state: 'blocked', - failedNames: ['broken.pdf'], - }); - - await render([]); expect(onFlush).toHaveBeenCalledTimes(1); expect(api.current!.armed).toBe(false); + + await render([att({ status: 'failed', fileName: 'broken.pdf' })]); + expect(onFlush).toHaveBeenCalledTimes(1); }); it('cancel disarms and prevents the auto-flush', async () => { diff --git a/frontend/src/components/message-input/armedSend.ts b/frontend/src/components/message-input/armedSend.ts index 6c64d711..34d8ef75 100644 --- a/frontend/src/components/message-input/armedSend.ts +++ b/frontend/src/components/message-input/armedSend.ts @@ -3,16 +3,11 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import type { Attachment } from '../../upload/uploadSlice'; export type SendReadiness = - | { state: 'ready' } - | { state: 'waiting'; pendingCount: number } - | { state: 'blocked'; failedNames: string[] }; + { state: 'ready' } | { state: 'waiting'; pendingCount: number }; +// A failed file never resolves, so it never holds the send: the composer +// drops it at submit time and sends the question with whatever succeeded. export function getSendReadiness(attachments: Attachment[]): SendReadiness { - const failedNames = attachments - .filter((a) => a.status === 'failed') - .map((a) => a.fileName); - if (failedNames.length > 0) return { state: 'blocked', failedNames }; - const pendingCount = attachments.filter( (a) => a.status === 'uploading' || a.status === 'processing', ).length; diff --git a/frontend/src/conversation/Conversation.tsx b/frontend/src/conversation/Conversation.tsx index ee06afda..0679c160 100644 --- a/frontend/src/conversation/Conversation.tsx +++ b/frontend/src/conversation/Conversation.tsx @@ -224,9 +224,9 @@ export default function Conversation() { }); } else if (getSendReadiness(attachments).state !== 'ready') { // Direct new sends (hero suggestion cards) bypass MessageInput's - // submit gate. With files still uploading/parsing (or failed), - // sending now would silently drop them — route the question into - // the composer instead, where the armed-send banner takes over. + // submit gate. With files still uploading/parsing, sending now + // would silently drop them — route the question into the composer + // instead, where the armed-send banner takes over. setQueuedQuestion(trimmedQuestion); } else { const filesAttached = completedAttachments @@ -247,7 +247,9 @@ export default function Conversation() { index, attachmentIds: filesAttached.map((f) => f.id), }); - if (filesAttached.length > 0) dispatch(clearAttachments()); + // Clears the sent files and drops any failed ones, which never + // hold a send (nothing is pending here, so nothing else remains). + if (attachments.length > 0) dispatch(clearAttachments()); } }, [dispatch, handleFetchAnswer, completedAttachments, attachments, queries], diff --git a/frontend/src/conversation/conversationSlice.test.ts b/frontend/src/conversation/conversationSlice.test.ts index 7ceeb0d4..47fab4cc 100644 --- a/frontend/src/conversation/conversationSlice.test.ts +++ b/frontend/src/conversation/conversationSlice.test.ts @@ -274,6 +274,24 @@ describe('fetchAnswer — attachment ids on the wire', () => { 'c-2', ]); }); + it('leaves a failed composer upload out of the fallback ids', async () => { + const store = makeStore(); + store.dispatch(addQuery({ prompt: 'q' })); + store.dispatch(addAttachment(completedAtt)); + store.dispatch( + addAttachment({ + id: 'f-3', + fileName: 'broken.pdf', + progress: 0, + status: 'failed', + taskId: '', + }), + ); + + await store.dispatch(fetchAnswer({ question: 'q', indx: 0 })); + + expect(vi.mocked(handleFetchAnswer).mock.calls[0][8]).toEqual(['srv-1']); + }); }); describe('fetchAnswer.rejected', () => { diff --git a/frontend/src/locale/en.json b/frontend/src/locale/en.json index 1d3e4a2a..fa295e00 100644 --- a/frontend/src/locale/en.json +++ b/frontend/src/locale/en.json @@ -1382,7 +1382,6 @@ "waitingToSend_one": "Will send when {{count}} file finishes processing…", "waitingToSend_other": "Will send when {{count}} files finish processing…", "cancelQueuedSend": "Cancel", - "sendBlockedByFailed": "{{names}} couldn't be processed — remove to send", "unsupportedType": "Not a supported file type ({{extension}})", "unreadableByModel": "{{model}} can't read {{name}} — switch to a model that supports images or PDFs", "attached": "Attachment",