diff --git a/frontend/src/components/MessageInput.test.tsx b/frontend/src/components/MessageInput.test.tsx new file mode 100644 index 00000000..ff658ed7 --- /dev/null +++ b/frontend/src/components/MessageInput.test.tsx @@ -0,0 +1,265 @@ +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'; +import { UPLOAD_STALL_TIMEOUT_MS } from './message-input/uploadStallGuard'; + +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, +}); + +/** Just enough of XMLHttpRequest for the composer's upload paths. */ +class FakeXHR extends EventTarget { + static instances: FakeXHR[] = []; + + upload = new EventTarget(); + status = 0; + responseText = ''; + timeout = 0; + onload: (() => void) | null = null; + onerror: (() => void) | null = null; + onabort: (() => void) | null = null; + ontimeout: (() => void) | null = null; + + open() {} + setRequestHeader() {} + send() { + FakeXHR.instances.push(this); + } + + private finish(handler: (() => void) | null, type: string) { + handler?.(); + this.dispatchEvent(new Event(type)); + this.dispatchEvent(new Event('loadend')); + } + + /** What Android Chrome reported: no response, status 0. */ + failNetwork() { + this.status = 0; + this.finish(this.onerror, 'error'); + } + + abort() { + this.finish(this.onabort, 'abort'); + } +} + +describe('MessageInput send with a failed attachment', () => { + let container: HTMLDivElement; + let root: Root; + let store: TestStore; + let onSubmit: ReturnType void>>; + const realXHR = globalThis.XMLHttpRequest; + + beforeEach(() => { + localStorage.clear(); + FakeXHR.instances = []; + globalThis.XMLHttpRequest = FakeXHR as unknown as typeof XMLHttpRequest; + 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(); + globalThis.XMLHttpRequest = realXHR; + vi.useRealTimers(); + }); + + 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 }), + ); + }); + }; + + const attachFile = async (name: string) => { + const input = container.querySelector( + 'label input[type="file"]', + )!; + const file = new File(['%PDF-1.4'], name, { type: 'application/pdf' }); + Object.defineProperty(input, 'files', { + value: [file], + configurable: true, + }); + const sent = FakeXHR.instances.length; + await act(async () => { + input.dispatchEvent(new Event('change', { bubbles: true })); + }); + // partitionAttachmentFiles sniffs the file asynchronously. + await act(async () => { + await vi.waitFor(() => + expect(FakeXHR.instances.length).toBeGreaterThan(sent), + ); + }); + }; + + 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('sends when the only attachment failed on the network', async () => { + await render(); + await attachFile('scan.pdf'); + expect(FakeXHR.instances).toHaveLength(1); + + await act(async () => FakeXHR.instances[0].failNetwork()); + const [chip] = store.getState().upload.attachments; + expect(chip.status).toBe('failed'); + expect(chip.errorMessage).toBe('conversation.attachments.uploadFailed'); + + await type('Summarise this'); + await pressEnter(); + + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit).toHaveBeenCalledWith('Summarise this'); + expect(store.getState().upload.attachments).toEqual([]); + }); + + 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(); + }); + + it('fails a stalled upload and then flushes the queued send', async () => { + vi.useFakeTimers(); + await render(); + await attachFile('huge.pdf'); + expect(store.getState().upload.attachments[0].status).toBe('uploading'); + + await type('Is it done?'); + await pressEnter(); + // Pending: the send is queued, not dropped. + expect(onSubmit).not.toHaveBeenCalled(); + + await act(async () => { + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS); + }); + + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit).toHaveBeenCalledWith('Is it done?'); + expect(store.getState().upload.attachments).toEqual([]); + }); + + it('keeps a queued question while another answer is streaming', async () => { + store.dispatch(addAttachment(att({ status: 'processing', progress: 30 }))); + await render(); + await type('Queued question'); + await pressEnter(); + expect(onSubmit).not.toHaveBeenCalled(); + + // A retry starts streaming, then the file settles. + await render(true); + await act(async () => { + store.dispatch({ + type: 'upload/updateAttachment', + payload: { id: 'ok-1', updates: { status: 'completed' } }, + }); + }); + + expect(onSubmit).not.toHaveBeenCalled(); + expect(textarea().value).toBe('Queued question'); + + // The stream ends: the held send goes out. + await render(false); + expect(onSubmit).toHaveBeenCalledTimes(1); + expect(onSubmit).toHaveBeenCalledWith('Queued question'); + }); +}); diff --git a/frontend/src/components/MessageInput.tsx b/frontend/src/components/MessageInput.tsx index c9a90216..1fd733df 100644 --- a/frontend/src/components/MessageInput.tsx +++ b/frontend/src/components/MessageInput.tsx @@ -51,6 +51,7 @@ import { ToolsTrigger, } from './message-input'; import { useArmedSend } from './message-input/armedSend'; +import { guardUploadStall } from './message-input/uploadStallGuard'; import { cannotReadAttachment } from './message-input/attachmentReadability'; import { handleAbort } from '../conversation/conversationSlice'; import { @@ -555,6 +556,7 @@ export default function MessageInput({ const files = supported; const apiHost = envVar('VITE_API_HOST'); + const uploadFailedMessage = t('conversation.attachments.uploadFailed'); if (files.length > 1) { const formData = new FormData(); @@ -830,20 +832,29 @@ export default function MessageInput({ } }; - xhr.onerror = () => { - console.error('Upload network error'); - Object.values(indexToUiId).forEach((id) => - dispatch( - updateAttachment({ - id, - updates: { status: 'failed' }, - }), - ), - ); - }; + // No response at all (status 0): a file the browser couldn't read, + // a dropped connection, or a stall the guard aborted. + xhr.onerror = + xhr.onabort = + xhr.ontimeout = + () => { + console.error('Upload network error'); + Object.values(indexToUiId).forEach((id) => + dispatch( + updateAttachment({ + id, + updates: { + status: 'failed', + errorMessage: uploadFailedMessage, + }, + }), + ), + ); + }; xhr.open('POST', `${apiHost}${endpoints.USER.STORE_ATTACHMENT}`); if (token) xhr.setRequestHeader('Authorization', `Bearer ${token}`); + guardUploadStall(xhr); xhr.send(formData); return; } @@ -951,17 +962,24 @@ export default function MessageInput({ } }; - xhr.onerror = () => { - dispatch( - updateAttachment({ - id: uniqueId, - updates: { status: 'failed' }, - }), - ); - }; + xhr.onerror = + xhr.onabort = + xhr.ontimeout = + () => { + dispatch( + updateAttachment({ + id: uniqueId, + updates: { + status: 'failed', + errorMessage: uploadFailedMessage, + }, + }), + ); + }; xhr.open('POST', `${apiHost}${endpoints.USER.STORE_ATTACHMENT}`); if (token) xhr.setRequestHeader('Authorization', `Bearer ${token}`); + guardUploadStall(xhr); xhr.send(formData); }); }, @@ -1578,17 +1596,30 @@ 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); - const canSubmit = - hasSubmittableContent && + Boolean(value.trim()) || + (allowSendWithoutText && attachments.some((a) => a.status !== 'failed')); + const composerIdle = !loading && recordingState !== 'recording' && recordingState !== 'transcribing'; + const canSubmit = hasSubmittableContent && composerIdle; 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) { @@ -1607,7 +1638,14 @@ export default function MessageInput({ readiness: sendReadiness, arm: armSend, cancel: cancelArmedSend, - } = useArmedSend({ attachments, onFlush: submitNow }); + } = useArmedSend({ + attachments, + onFlush: submitNow, + // A queued send that settles while another answer streams would be + // refused by the consumer after the composer was already cleared; hold + // it until the composer can take a submit again. + canFlush: composerIdle, + }); // Adopt a question queued outside the composer: seed the input, arm, // and hand the wait to the standard banner. If the attachments already @@ -1621,9 +1659,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 +1761,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/AttachmentChipList.test.tsx b/frontend/src/components/message-input/AttachmentChipList.test.tsx new file mode 100644 index 00000000..df8a5a1d --- /dev/null +++ b/frontend/src/components/message-input/AttachmentChipList.test.tsx @@ -0,0 +1,122 @@ +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +import type { Attachment } from '../../upload/uploadSlice'; +import AttachmentChipList from './AttachmentChipList'; + +Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + +const att = (over: Partial = {}): Attachment => ({ + id: 'a1', + fileName: 'scan.pdf', + progress: 0, + status: 'failed', + taskId: '', + errorMessage: 'Upload failed. The file could not be read.', + ...over, +}); + +describe('AttachmentChipList failed chip', () => { + let container: HTMLDivElement; + let root: Root; + + beforeEach(() => { + vi.useFakeTimers(); + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(async () => { + await act(async () => root.unmount()); + container.remove(); + vi.useRealTimers(); + }); + + const render = async (attachments: Attachment[]) => { + await act(async () => { + root.render( + {}} + onDragStart={() => {}} + onDragOver={() => {}} + onDropOn={() => {}} + />, + ); + }); + }; + + const tooltip = () => + document.body.querySelector('[data-slot="tooltip-content"]'); + + it('shows no inline failure line under the chips', async () => { + await render([att()]); + + expect(container.querySelector('[role="alert"]')).toBeNull(); + expect(container.querySelector('.text-destructive')).toBeNull(); + }); + + it('shows the reason in a tooltip when the chip is hovered', async () => { + await render([att()]); + expect(tooltip()).toBeNull(); + + // The remove X is an IconButton with its own tooltip; the chip is the div. + const chip = container.querySelector('div[data-slot="tooltip-trigger"]')!; + await act(async () => { + chip.dispatchEvent( + new PointerEvent('pointermove', { + bubbles: true, + pointerType: 'mouse', + }), + ); + vi.advanceTimersByTime(500); + }); + + // The full name, since two long names can truncate to the same text. + expect(tooltip()?.textContent).toContain( + 'scan.pdf: Upload failed. The file could not be read.', + ); + }); + + it('announces failures in a persistent polite live region', async () => { + await render([att({ status: 'uploading', errorMessage: undefined })]); + const region = container.querySelector('[role="status"]'); + expect(region?.textContent).toBe(''); + + await render([att()]); + + // Same node, updated in place, so screen readers announce the change. + expect(container.querySelector('[role="status"]')).toBe(region); + expect(region?.textContent).toBe( + 'scan.pdf: Upload failed. The file could not be read.', + ); + }); + + it('describes the focusable remove button with the reason', async () => { + await render([att()]); + + const remove = container.querySelector( + 'button[aria-label="conversation.attachments.remove"]', + )!; + const describedBy = remove.getAttribute('aria-describedby'); + expect(describedBy).toBeTruthy(); + // Named, since the button's own label doesn't say which file. + expect(document.getElementById(describedBy!)?.textContent).toBe( + 'scan.pdf: Upload failed. The file could not be read.', + ); + }); + + it('adds no tooltip to a chip that did not fail', async () => { + await render([att({ status: 'completed', errorMessage: undefined })]); + + expect( + container.querySelector('div[data-slot="tooltip-trigger"]'), + ).toBeNull(); + }); +}); diff --git a/frontend/src/components/message-input/AttachmentChipList.tsx b/frontend/src/components/message-input/AttachmentChipList.tsx index 7d31dfc9..6376e7fd 100644 --- a/frontend/src/components/message-input/AttachmentChipList.tsx +++ b/frontend/src/components/message-input/AttachmentChipList.tsx @@ -1,8 +1,10 @@ import { Paperclip, TriangleAlert, X } from 'lucide-react'; +import { useId } from 'react'; import { useTranslation } from 'react-i18next'; import type { Attachment } from '../../upload/uploadSlice'; import { IconButton } from '../ui/icon-button'; +import { Tooltip, TooltipContent, TooltipTrigger } from '../ui/tooltip'; import { cn } from '@/lib/utils'; type AttachmentChipListProps = { @@ -29,13 +31,10 @@ export default function AttachmentChipList({ modelName, }: AttachmentChipListProps) { const { t } = useTranslation(); + const reasonIdPrefix = useId(); + const failureReasonOf = (attachment: Attachment) => + attachment.errorMessage ?? t('conversation.attachments.failed'); - // A tooltip is the one place a touch user can never look, and this list is - // where a phone picker's unsupported file lands. Show the reason inline, - // as soon as it is known, rather than only once a send is attempted. - const failures = attachments.filter( - (attachment) => attachment.status === 'failed' && attachment.errorMessage, - ); // Not a failure: the file is kept and still sends. It only warns that the // model picked right now would receive nothing it can read. const unreadable = modelName @@ -46,7 +45,14 @@ export default function AttachmentChipList({ <>
{attachments.map((attachment) => { - return ( + const failed = attachment.status === 'failed'; + // A failed file never blocks the send (it is dropped then), so its + // reason stays out of the way: the chip's warning icon marks it, the + // reason is a hover tooltip, and it describes the remove button (the + // chip's one focusable control) for keyboard and screen readers. + const failureReason = failed ? failureReasonOf(attachment) : null; + const reasonId = `${reasonIdPrefix}-${attachment.id}`; + const chip = (
{attachment.status === 'completed' && ( @@ -115,9 +116,17 @@ export default function AttachmentChipList({ )}
- + {attachment.fileName} + {failureReason && ( + + )} { onRemove(attachment.id); }} @@ -133,21 +143,29 @@ export default function AttachmentChipList({
); + if (!failureReason) return chip; + return ( + + {chip} + + {attachment.fileName}: {failureReason} + + + ); })}
- {failures.length > 0 && ( -
- {failures.map((attachment) => ( - - {attachment.fileName}: {attachment.errorMessage} - - ))} -
- )} + {/* Always mounted, so a chip turning failed is announced: the reason + itself is only on hover, which a screen reader never sees. */} +
+ {attachments + .filter((attachment) => attachment.status === 'failed') + .map( + (attachment) => + `${attachment.fileName}: ${failureReasonOf(attachment)}`, + ) + .join(' ')} +
{unreadable.length > 0 && (
{ ).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 }); }); }); @@ -60,13 +66,15 @@ type HookApi = ReturnType; function Host({ attachments, onFlush, + canFlush, api, }: { attachments: Attachment[]; onFlush: () => void; + canFlush?: boolean; api: { current: HookApi | null }; }) { - const hook = useArmedSend({ attachments, onFlush }); + const hook = useArmedSend({ attachments, onFlush, canFlush }); api.current = hook; return null; } @@ -90,10 +98,15 @@ describe('useArmedSend', () => { container.remove(); }); - const render = async (attachments: Attachment[]) => { + const render = async (attachments: Attachment[], canFlush?: boolean) => { await act(async () => { root.render( - , + , ); }); }; @@ -120,19 +133,30 @@ describe('useArmedSend', () => { expect(onFlush).toHaveBeenCalledTimes(1); }); - it('holds the flush while a file is failed and resumes when it is removed', async () => { + 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).toHaveBeenCalledTimes(1); + expect(api.current!.armed).toBe(false); + + await render([att({ status: 'failed', fileName: 'broken.pdf' })]); + expect(onFlush).toHaveBeenCalledTimes(1); + }); + + it('holds a ready flush until canFlush turns true', async () => { await render([att({ status: 'processing' })]); await act(async () => api.current!.arm()); - await render([att({ status: 'failed', fileName: 'broken.pdf' })]); + // Files resolved, but another answer is streaming: flushing now would + // clear the composer while the submit is refused. + await render([att()], false); expect(onFlush).not.toHaveBeenCalled(); expect(api.current!.armed).toBe(true); - expect(api.current!.readiness).toEqual({ - state: 'blocked', - failedNames: ['broken.pdf'], - }); - await render([]); + await render([att()], true); expect(onFlush).toHaveBeenCalledTimes(1); expect(api.current!.armed).toBe(false); }); diff --git a/frontend/src/components/message-input/armedSend.ts b/frontend/src/components/message-input/armedSend.ts index 6c64d711..49cf712a 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; @@ -24,9 +19,13 @@ export function getSendReadiness(attachments: Attachment[]): SendReadiness { export function useArmedSend({ attachments, onFlush, + canFlush = true, }: { attachments: Attachment[]; onFlush: () => void; + // False while the composer can't take a submit (an answer is streaming): + // the armed send holds until it can, instead of flushing into a refusal. + canFlush?: boolean; }) { const [armed, setArmed] = useState(false); // Latest-closure ref so the flush submits the current composer value, @@ -42,12 +41,12 @@ export function useArmedSend({ flushedRef.current = false; return; } - if (readiness.state === 'ready' && !flushedRef.current) { + if (readiness.state === 'ready' && canFlush && !flushedRef.current) { flushedRef.current = true; setArmed(false); flushRef.current(); } - }, [armed, readiness.state]); + }, [armed, readiness.state, canFlush]); const arm = useCallback(() => setArmed(true), []); const cancel = useCallback(() => setArmed(false), []); diff --git a/frontend/src/components/message-input/uploadStallGuard.test.ts b/frontend/src/components/message-input/uploadStallGuard.test.ts new file mode 100644 index 00000000..02262b5e --- /dev/null +++ b/frontend/src/components/message-input/uploadStallGuard.test.ts @@ -0,0 +1,54 @@ +import { guardUploadStall, UPLOAD_STALL_TIMEOUT_MS } from './uploadStallGuard'; + +class FakeXHR extends EventTarget { + upload = new EventTarget(); + abort = vi.fn(() => { + this.dispatchEvent(new Event('abort')); + this.dispatchEvent(new Event('loadend')); + }); +} + +const make = () => { + const xhr = new FakeXHR(); + guardUploadStall(xhr as unknown as XMLHttpRequest); + return xhr; +}; + +describe('guardUploadStall', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('aborts an upload that never reports progress', () => { + const xhr = make(); + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS - 1); + expect(xhr.abort).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(xhr.abort).toHaveBeenCalledTimes(1); + }); + + it('lets a slow upload run while it keeps making progress', () => { + const xhr = make(); + for (let i = 0; i < 5; i += 1) { + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS - 1); + xhr.upload.dispatchEvent(new Event('progress')); + } + expect(xhr.abort).not.toHaveBeenCalled(); + }); + + it('gives the server a fresh window once the body is sent', () => { + const xhr = make(); + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS - 1); + xhr.upload.dispatchEvent(new Event('load')); + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS - 1); + expect(xhr.abort).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(xhr.abort).toHaveBeenCalledTimes(1); + }); + + it('stops watching once the request settles', () => { + const xhr = make(); + xhr.dispatchEvent(new Event('loadend')); + vi.advanceTimersByTime(UPLOAD_STALL_TIMEOUT_MS * 2); + expect(xhr.abort).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/components/message-input/uploadStallGuard.ts b/frontend/src/components/message-input/uploadStallGuard.ts new file mode 100644 index 00000000..bf79cad5 --- /dev/null +++ b/frontend/src/components/message-input/uploadStallGuard.ts @@ -0,0 +1,33 @@ +// How long an attachment upload may go without any sign of life. It is an +// idle window, not a total cap: a large file on a slow link keeps resetting +// it with progress events, so only a silent stall is cut off. +export const UPLOAD_STALL_TIMEOUT_MS = 120_000; + +/** + * Abort ``xhr`` once it makes no progress for ``idleMs``. + * + * ``xhr.timeout`` would cap the whole request, which a large file on a slow + * connection can legitimately exceed. This resets on every upload progress + * event, and again when the body finishes sending so the server gets its own + * window to answer. The abort fires the request's ``onabort`` handler, which + * is where the caller marks the attachment failed. + * + * Args: + * xhr: The request to watch. Call before ``send()``. + * idleMs: The longest allowed gap between signs of progress. + */ +export function guardUploadStall( + xhr: XMLHttpRequest, + idleMs: number = UPLOAD_STALL_TIMEOUT_MS, +): void { + let timer: ReturnType | undefined; + const restart = () => { + clearTimeout(timer); + timer = setTimeout(() => xhr.abort(), idleMs); + }; + xhr.upload.addEventListener('progress', restart); + xhr.upload.addEventListener('load', restart); + xhr.addEventListener('progress', restart); + xhr.addEventListener('loadend', () => clearTimeout(timer)); + restart(); +} 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/de.json b/frontend/src/locale/de.json index e788de44..7c35fe9b 100644 --- a/frontend/src/locale/de.json +++ b/frontend/src/locale/de.json @@ -1373,6 +1373,7 @@ "attachments": { "attach": "Anhängen", "remove": "Anhang entfernen", + "uploadFailed": "Upload fehlgeschlagen. Die Datei konnte nicht gelesen werden oder die Verbindung wurde unterbrochen.", "attached": "Anhang", "failed": "Fehlgeschlagen", "attachment": "Anhang" diff --git a/frontend/src/locale/en.json b/frontend/src/locale/en.json index 1d3e4a2a..986577a1 100644 --- a/frontend/src/locale/en.json +++ b/frontend/src/locale/en.json @@ -1379,10 +1379,10 @@ "attachments": { "attach": "Attach", "remove": "Remove attachment", + "uploadFailed": "Upload failed. The file couldn't be read or the connection dropped.", "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", diff --git a/frontend/src/locale/es.json b/frontend/src/locale/es.json index 90b38b89..681e2e0d 100644 --- a/frontend/src/locale/es.json +++ b/frontend/src/locale/es.json @@ -1373,6 +1373,7 @@ "attachments": { "attach": "Adjuntar", "remove": "Eliminar adjunto", + "uploadFailed": "Error al subir. No se pudo leer el archivo o se perdió la conexión.", "attached": "Adjunto", "failed": "Error", "attachment": "Adjunto" diff --git a/frontend/src/locale/jp.json b/frontend/src/locale/jp.json index 41cf29f1..9e0fa1dd 100644 --- a/frontend/src/locale/jp.json +++ b/frontend/src/locale/jp.json @@ -1370,6 +1370,7 @@ "attachments": { "attach": "添付", "remove": "添付ファイルを削除", + "uploadFailed": "アップロードに失敗しました。ファイルを読み取れなかったか、接続が切断されました。", "attached": "添付ファイル", "failed": "失敗", "attachment": "添付ファイル" diff --git a/frontend/src/locale/ru.json b/frontend/src/locale/ru.json index 7a9d0352..c00face0 100644 --- a/frontend/src/locale/ru.json +++ b/frontend/src/locale/ru.json @@ -1417,6 +1417,7 @@ "attachments": { "attach": "Прикрепить", "remove": "Удалить вложение", + "uploadFailed": "Не удалось загрузить. Файл не удалось прочитать или соединение прервалось.", "attached": "Вложение", "failed": "Ошибка", "attachment": "Вложение" diff --git a/frontend/src/locale/zh-TW.json b/frontend/src/locale/zh-TW.json index ff1d2efc..3ca0570b 100644 --- a/frontend/src/locale/zh-TW.json +++ b/frontend/src/locale/zh-TW.json @@ -1370,6 +1370,7 @@ "attachments": { "attach": "附件", "remove": "刪除附件", + "uploadFailed": "上傳失敗。無法讀取檔案或連線已中斷。", "attached": "附件", "failed": "失敗", "attachment": "附件" diff --git a/frontend/src/locale/zh.json b/frontend/src/locale/zh.json index 36dc4e58..d41815c1 100644 --- a/frontend/src/locale/zh.json +++ b/frontend/src/locale/zh.json @@ -1370,6 +1370,7 @@ "attachments": { "attach": "附件", "remove": "删除附件", + "uploadFailed": "上传失败。无法读取文件或连接已断开。", "attached": "附件", "failed": "失败", "attachment": "附件" diff --git a/tests/e2e/specs/tier-a/composer-resilience.spec.ts b/tests/e2e/specs/tier-a/composer-resilience.spec.ts index 5bdc9f39..146f001a 100644 --- a/tests/e2e/specs/tier-a/composer-resilience.spec.ts +++ b/tests/e2e/specs/tier-a/composer-resilience.spec.ts @@ -14,6 +14,10 @@ * file arrives bound to the turn (`conversation_messages.attachments[]`) * instead of being silently dropped from the payload. * + * 3. Send with an attachment that failed. A failed file never resolves, + * so it must never hold the question: Send drops the failed chip and + * sends the text with whatever did upload. + * * UI-driven on purpose: the surface under test IS the composer. The * attachments spec avoids the file picker for API-direct upload tests; * here `setInputFiles` on the dropzone input is the point. @@ -246,4 +250,77 @@ test.describe("tier-a · composer resilience", () => { await context.close(); } }); + + test("send with a failed attachment drops the file and sends the question", async ({ + browser, + }) => { + const { context, sub } = await newUserContext(browser); + try { + const page = await context.newPage(); + await openComposer(page); + + const streamPosts: string[] = []; + page.on("request", (req) => { + if (req.url().includes("/stream") && req.method() === "POST") { + streamPosts.push(req.postData() ?? ""); + } + }); + + // No response at all (status 0), the shape of the Android file-read + // failure seen in production. + await page.route("**/api/store_attachment", (route) => + route.abort("failed"), + ); + await page + .locator('input[type="file"]') + .first() + .setInputFiles(SMALL_FIXTURE_PATH); + + // The failed chip is marked by its warning icon; the reason is a + // hover tooltip, not a line of text in the composer. + const failedIcon = page.getByLabel("Failed", { exact: true }); + await expect(failedIcon).toBeVisible({ timeout: 5_000 }); + await failedIcon.hover(); + await expect(page.getByRole("tooltip")).toContainText(/upload failed/i); + + const question = "does this still send? e2e-failed-attachment"; + const textarea = page.locator("#message-input"); + await textarea.fill(question); + const streamDone = page.waitForResponse( + (r) => r.url().includes("/stream") && r.request().method() === "POST", + { timeout: 30_000 }, + ); + await textarea.press("Enter"); + expect((await streamDone).status()).toBe(200); + + // Exactly one send, with no attachment ids. + expect(streamPosts).toHaveLength(1); + const payload = JSON.parse(streamPosts[0]) as { attachments?: string[] }; + expect(payload.attachments ?? []).toHaveLength(0); + + // The question is in the thread; the chip and its reason are gone. + await expect(page.getByText(question)).toBeVisible(); + await expect(failedIcon).toHaveCount(0); + await expect(page.getByText(/upload failed/i)).toHaveCount(0); + await expect(page.getByText("notes.txt")).toBeHidden(); + await expect(textarea).toHaveValue(""); + + const { rows } = await pg.query<{ + prompt: string | null; + attachment_count: number | string | null; + }>( + `SELECT cm.prompt, + COALESCE(array_length(cm.attachments, 1), 0) AS attachment_count + FROM conversation_messages cm + JOIN conversations c ON c.id = cm.conversation_id + WHERE c.user_id = $1`, + [sub], + ); + expect(rows).toHaveLength(1); + expect(rows[0].prompt).toBe(question); + expect(Number(rows[0].attachment_count)).toBe(0); + } finally { + await context.close(); + } + }); });