diff --git a/client/src/components/Chat/Input/__tests__/ChatForm.attachments.spec.tsx b/client/src/components/Chat/Input/__tests__/ChatForm.attachments.spec.tsx new file mode 100644 index 0000000000..dc21d840f2 --- /dev/null +++ b/client/src/components/Chat/Input/__tests__/ChatForm.attachments.spec.tsx @@ -0,0 +1,226 @@ +import React, { Profiler, useMemo, useState } from 'react'; +import '@testing-library/jest-dom'; +import { DndProvider } from 'react-dnd'; +import { useForm } from 'react-hook-form'; +import { RecoilRoot, useRecoilState } from 'recoil'; +import userEvent from '@testing-library/user-event'; +import { HTML5Backend } from 'react-dnd-html5-backend'; +import { BrowserRouter as Router } from 'react-router-dom'; +import { render, screen, waitFor } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { QueryKeys, FileSources, EModelEndpoint } from 'librechat-data-provider'; +import type { TFile, TFileUpload, TConversation } from 'librechat-data-provider'; +import type { ChatFormValues } from '~/common'; +import { ChatContext, ChatFormProvider } from '~/Providers'; +import { AuthContextProvider } from '~/hooks/AuthContext'; +import ChatForm from '../ChatForm'; +import store from '~/store'; + +const mockUpload = jest.fn(); + +jest.mock('librechat-data-provider', () => { + const actual = jest.requireActual('librechat-data-provider'); + return { + ...actual, + dataService: { + ...actual.dataService, + uploadImage: (...args: unknown[]) => mockUpload(...args), + uploadFile: (...args: unknown[]) => mockUpload(...args), + }, + }; +}); + +const conversation = { + conversationId: 'new', + endpoint: EModelEndpoint.openAI, + model: 'gpt-4o', + title: 'New Chat', +} as TConversation; + +const uploadResponse = { + message: 'File uploaded', + file_id: 'server-file-id', + temp_file_id: 'temp-file-id', + filename: 'cat.png', + filepath: '/images/cat.png', + type: 'image/png', + bytes: 2048, + height: 100, + width: 100, + source: FileSources.local, + embedded: false, +} as unknown as TFileUpload; + +/** jsdom never decodes images; `decodes` mirrors a browser that can or cannot. */ +let decodes = true; + +class StubImage { + onload: (() => void) | null = null; + onerror: (() => void) | null = null; + width = 100; + height = 100; + set src(_value: string) { + setTimeout(() => (decodes ? this.onload?.() : this.onerror?.()), 0); + } +} + +let commits = 0; + +function Harness() { + const [files, setFiles] = useRecoilState(store.filesByIndex(0)); + const [isSubmitting] = useRecoilState(store.isSubmittingFamily(0)); + const [, setFilesLoading] = useState(false); + const methods = useForm({ defaultValues: { text: '' } }); + + const chatHelpers = useMemo( + () => + ({ + index: 0, + conversation, + setConversation: () => undefined, + files, + setFiles, + isSubmitting, + setIsSubmitting: () => undefined, + filesLoading: false, + setFilesLoading, + newConversation: () => undefined, + handleStopGenerating: () => undefined, + stopGenerating: () => undefined, + getMessages: () => undefined, + setMessages: () => undefined, + ask: () => undefined, + regenerate: () => undefined, + setSiblingIdx: () => undefined, + showPopover: false, + setShowPopover: () => undefined, + abortScroll: false, + setAbortScroll: () => undefined, + preset: null, + setPreset: () => undefined, + optionSettings: {}, + setOptionSettings: () => undefined, + handleRegenerate: () => undefined, + handleContinue: () => undefined, + }) as unknown as React.ContextType, + [files, setFiles, isSubmitting], + ); + + return ( + + + (commits += 1)}> + + + + + ); +} + +function renderComposer() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + queryClient.setQueryData([QueryKeys.fileConfig], {}); + queryClient.setQueryData([QueryKeys.files], []); + queryClient.setQueryData([QueryKeys.endpoints], { [EModelEndpoint.openAI]: { order: 0 } }); + + return render( + + + + + + + + + + + , + ); +} + +const sendButton = () => screen.getByTestId('send-button'); +const attach = (container: HTMLElement, file: File) => + userEvent.upload(container.querySelector('input[type="file"]') as HTMLInputElement, file); +const image = () => new File(['image-bytes'], 'cat.png', { type: 'image/png' }); + +describe('ChatForm attachments', () => { + beforeEach(() => { + localStorage.clear(); + decodes = true; + commits = 0; + global.URL.createObjectURL = jest.fn(() => 'blob:preview'); + global.URL.revokeObjectURL = jest.fn(); + (global as unknown as { Image: unknown }).Image = StubImage; + mockUpload.mockReset(); + /** The server echoes the id the client sent back as `temp_file_id`. */ + mockUpload.mockImplementation((body: FormData) => + Promise.resolve({ ...uploadResponse, temp_file_id: body.get('file_id') as string }), + ); + }); + + test('re-enables send once an attachment finishes uploading', async () => { + const { container } = renderComposer(); + + const textarea = await screen.findByTestId('text-input'); + await userEvent.type(textarea, 'hi'); + expect(sendButton()).toBeEnabled(); + + await attach(container, image()); + await waitFor(() => expect(mockUpload).toHaveBeenCalled()); + + await waitFor(() => expect(sendButton()).toBeEnabled()); + expect(textarea).toHaveValue('hi'); + }, 20000); + + test('enables send for an attachment with no composer text', async () => { + const { container } = renderComposer(); + await screen.findByTestId('text-input'); + expect(sendButton()).toBeDisabled(); + + await attach(container, image()); + await waitFor(() => expect(mockUpload).toHaveBeenCalled()); + + await waitFor(() => expect(sendButton()).toBeEnabled()); + }, 20000); + + /** + * The upload only starts once the browser has decoded the image. A decode it + * refuses used to leave the attachment below `progress: 1`, which reads as + * "still uploading" and disabled the send button for the rest of the session. + */ + test('drops an image the browser cannot decode instead of disabling send', async () => { + decodes = false; + const { container } = renderComposer(); + + const textarea = await screen.findByTestId('text-input'); + await userEvent.type(textarea, 'hi'); + + await attach(container, image()); + await waitFor(() => expect(screen.queryByLabelText('Remove file')).not.toBeInTheDocument()); + + expect(mockUpload).not.toHaveBeenCalled(); + expect(sendButton()).toBeEnabled(); + expect(textarea).toHaveValue('hi'); + }, 20000); + + /** + * The composer is the app's busiest surface: every keystroke already re-renders + * it for the row count and the send button's enabled state, so anything that + * multiplies that work per character is a regression worth failing on. The + * measured cost is ~2.5 commits per character (react-scan reports one ChatForm + * render per keystroke in a real browser); the bound leaves headroom for jsdom + * scheduling without tolerating a doubling. + */ + test('keeps typing render-bounded', async () => { + renderComposer(); + const textarea = await screen.findByTestId('text-input'); + await waitFor(() => expect(sendButton()).toBeInTheDocument()); + + commits = 0; + await userEvent.type(textarea, 'hello there'); + + expect(commits).toBeLessThanOrEqual('hello there'.length * 3); + }, 20000); +}); diff --git a/client/src/data-provider/Files/__tests__/uploadNormalization.spec.tsx b/client/src/data-provider/Files/__tests__/uploadNormalization.spec.tsx new file mode 100644 index 0000000000..92326598ab --- /dev/null +++ b/client/src/data-provider/Files/__tests__/uploadNormalization.spec.tsx @@ -0,0 +1,115 @@ +import React from 'react'; +import { QueryKeys, FileSources } from 'librechat-data-provider'; +import { renderHook, act, waitFor } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type { TFile, TFileUpload } from 'librechat-data-provider'; + +const mockUploadFile = jest.fn(); + +jest.mock('librechat-data-provider', () => { + const actual = jest.requireActual('librechat-data-provider'); + return { + ...actual, + dataService: { + ...actual.dataService, + uploadFile: (...args: unknown[]) => mockUploadFile(...args), + uploadImage: (...args: unknown[]) => mockUploadFile(...args), + }, + }; +}); + +jest.mock('../../Endpoints', () => ({ + useGetStartupConfig: () => ({ data: undefined }), +})); + +import { useUploadFileMutation } from '../mutations'; + +const REQUEST_FILE_ID = 'client-request-id'; + +const uploadResponse = { + file_id: 'server-file-id', + temp_file_id: 'an-id-the-client-never-sent', + filename: 'notes.txt', + filepath: '/files/notes.txt', + type: 'text/plain', + bytes: 12, + object: 'file', + usage: 0, + user: 'user-1', + embedded: false, + source: FileSources.local, +} as unknown as TFileUpload; + +const body = () => { + const formData = new FormData(); + formData.append('file_id', REQUEST_FILE_ID); + formData.append('message_file', 'true'); + return formData; +}; + +/** + * The composer keys its file map, and the draft it saves, by the `file_id` the + * request was sent with; `temp_file_id` is only the server's echo of that id. + * A cached record carrying an echo that disagrees cannot be correlated back to + * the draft, so `useAutoSave` cannot restore the attachment after a conversation + * switch or a reload. + */ +describe('useUploadFileMutation — temporary id normalization', () => { + let queryClient: QueryClient; + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} + ); + + beforeEach(() => { + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + mockUploadFile.mockReset(); + mockUploadFile.mockResolvedValue(uploadResponse); + }); + + test('caches the uploaded record under the id the request was sent with', async () => { + const { result } = renderHook(() => useUploadFileMutation(), { wrapper }); + + act(() => { + result.current.mutate(body()); + }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + const [cached] = queryClient.getQueryData([QueryKeys.files]) ?? []; + expect(cached).toMatchObject({ + file_id: 'server-file-id', + temp_file_id: REQUEST_FILE_ID, + }); + }); + + test('hands the normalized record to the caller', async () => { + const onSuccess = jest.fn(); + const { result } = renderHook(() => useUploadFileMutation({ onSuccess }), { wrapper }); + + act(() => { + result.current.mutate(body()); + }); + + await waitFor(() => expect(onSuccess).toHaveBeenCalled()); + + expect(onSuccess.mock.calls[0][0]).toMatchObject({ temp_file_id: REQUEST_FILE_ID }); + }); + + test('leaves an agreeing response untouched', async () => { + mockUploadFile.mockResolvedValue({ ...uploadResponse, temp_file_id: REQUEST_FILE_ID }); + const onSuccess = jest.fn(); + const { result } = renderHook(() => useUploadFileMutation({ onSuccess }), { wrapper }); + + act(() => { + result.current.mutate(body()); + }); + + await waitFor(() => expect(onSuccess).toHaveBeenCalled()); + + const [cached] = queryClient.getQueryData([QueryKeys.files]) ?? []; + expect(cached).toBe(onSuccess.mock.calls[0][0]); + }); +}); diff --git a/client/src/data-provider/Files/mutations.ts b/client/src/data-provider/Files/mutations.ts index a538eea4b8..097829022a 100644 --- a/client/src/data-provider/Files/mutations.ts +++ b/client/src/data-provider/Files/mutations.ts @@ -44,8 +44,18 @@ export const useUploadFileMutation = ( }, ...options, onSuccess: (data, formData, context) => { + /** `temp_file_id` is the server's echo of the `file_id` the request was sent + * with, and that request id is what every client-side handle for the upload + * is keyed by — the composer's file map, the draft it saves, the delayed + * toast timer. A record cached under an echo that disagrees cannot be + * correlated back to the draft, so the attachment is silently dropped on the + * next conversation switch or reload. Normalize once, on the way in. */ + const requestFileId = (formData.get('file_id') as string | null) ?? data.temp_file_id; + const file = + data.temp_file_id === requestFileId ? data : { ...data, temp_file_id: requestFileId }; + queryClient.setQueryData([QueryKeys.files], (_files) => [ - data, + file, ...(_files ?? []), ]); @@ -56,7 +66,7 @@ export const useUploadFileMutation = ( const tool_resource = (formData.get('tool_resource') as string | undefined) ?? ''; if (message_file === 'true') { - onSuccess?.(data, formData, context); + onSuccess?.(file, formData, context); return; } @@ -92,7 +102,7 @@ export const useUploadFileMutation = ( } if (!assistant_id) { - onSuccess?.(data, formData, context); + onSuccess?.(file, formData, context); return; } @@ -136,7 +146,7 @@ export const useUploadFileMutation = ( }; }, ); - onSuccess?.(data, formData, context); + onSuccess?.(file, formData, context); }, }); }; diff --git a/client/src/hooks/Files/__tests__/useFileHandling.test.ts b/client/src/hooks/Files/__tests__/useFileHandling.test.ts index c643b4b9f4..0378f58b04 100644 --- a/client/src/hooks/Files/__tests__/useFileHandling.test.ts +++ b/client/src/hooks/Files/__tests__/useFileHandling.test.ts @@ -8,20 +8,26 @@ import { } from 'librechat-data-provider'; type MockUploadMutationOptions = { - onSuccess?: (data: { - temp_file_id: string; - file_id: string; - filepath: string; - type: string; - filename: string; - source: string; - embedded: boolean; - height?: number; - width?: number; - }) => void; + onSuccess?: ( + data: { + temp_file_id: string; + file_id: string; + filepath: string; + type: string; + filename: string; + source: string; + embedded: boolean; + height?: number; + width?: number; + }, + body: FormData, + ) => void; onError?: (error: unknown, body: FormData) => void; }; +/** Mirrors a browser that can (or cannot) decode the bytes it was handed. */ +let mockImageDecodes = true; + beforeAll(() => { global.URL.createObjectURL = jest.fn(() => 'blob:mock-url'); global.URL.revokeObjectURL = jest.fn(); @@ -31,9 +37,10 @@ beforeAll(() => { width = 640; height = 480; onload: (() => void) | null = null; + onerror: (() => void) | null = null; set src(_src: string) { - queueMicrotask(() => this.onload?.()); + queueMicrotask(() => (mockImageDecodes ? this.onload?.() : this.onerror?.())); } }, }); @@ -131,13 +138,18 @@ jest.mock('../useClientResize', () => ({ })), })); +const mockAddFile = jest.fn(); +const mockReplaceFile = jest.fn(); +const mockUpdateFileById = jest.fn(); +const mockDeleteFileById = jest.fn(); + jest.mock('../useUpdateFiles', () => ({ __esModule: true, default: jest.fn(() => ({ - addFile: jest.fn(), - replaceFile: jest.fn(), - updateFileById: jest.fn(), - deleteFileById: jest.fn(), + addFile: mockAddFile, + replaceFile: mockReplaceFile, + updateFileById: mockUpdateFileById, + deleteFileById: mockDeleteFileById, })), })); @@ -173,6 +185,7 @@ describe('useFileHandling', () => { mockFileConfig = null; mockIsConfigPending = false; mockIsTemporary = false; + mockImageDecodes = true; mockUploadOptions = {}; }); @@ -1057,15 +1070,18 @@ describe('useFileHandling', () => { expect(onStart).toHaveBeenCalledWith(fileId); act(() => { - mockUploadOptions.onSuccess?.({ - temp_file_id: fileId, - file_id: 'saved-file-id', - filepath: '/files/notes.txt', - type: 'text/plain', - filename: 'notes.txt', - source: 'local', - embedded: false, - }); + mockUploadOptions.onSuccess?.( + { + temp_file_id: fileId, + file_id: 'saved-file-id', + filepath: '/files/notes.txt', + type: 'text/plain', + filename: 'notes.txt', + source: 'local', + embedded: false, + }, + uploadBody, + ); }); expect(onSuccess).toHaveBeenCalledWith(fileId); @@ -1160,4 +1176,169 @@ describe('useFileHandling', () => { consoleLog.mockRestore(); }); }); + describe('stalled attachments', () => { + /** + * The upload only starts once the browser has decoded the image, so a decode + * it refuses must not leave the attachment parked below `progress: 1` — the + * composer reads that as "still uploading" and disables send for the rest of + * the session. + */ + it('drops an image the browser cannot decode instead of parking it mid-upload', async () => { + mockImageDecodes = false; + const useFileHandling = await loadHook(); + const { result } = renderHook(() => useFileHandling()); + + await act(async () => { + await result.current.handleFiles([ + new File(['broken'], 'photo.png', { type: 'image/png' }), + ]); + }); + await act(async () => { + await Promise.resolve(); + }); + + const addedFile = mockAddFile.mock.calls[0][0] as { file_id: string }; + expect(mockMutate).not.toHaveBeenCalled(); + expect(mockDeleteFileById).toHaveBeenCalledWith(addedFile.file_id); + }); + + it('reports the failure through the upload lifecycle when a decode fails', async () => { + mockImageDecodes = false; + const onError = jest.fn(); + const useFileHandling = await loadHook(); + const { result } = renderHook(() => useFileHandling()); + + await act(async () => { + await result.current.handleFiles( + [new File(['broken'], 'photo.png', { type: 'image/png' })], + undefined, + { fileId: 'pending-paste-id', onError }, + ); + }); + await act(async () => { + await Promise.resolve(); + }); + + expect(onError).toHaveBeenCalledWith('pending-paste-id'); + }); + + /** + * Every client-side handle for an upload is keyed by the id the request was + * sent with; `temp_file_id` is only the server's echo of it. Reconciling + * against the echo strands the attachment when the two disagree. + */ + it('keys the completed attachment temporary id to the id the request was sent with', async () => { + jest.useFakeTimers(); + const consoleLog = jest.spyOn(console, 'log').mockImplementation(() => undefined); + const useFileHandling = await loadHook(); + const { result } = renderHook(() => useFileHandling()); + + await act(async () => { + await result.current.handleFiles([ + new File(['hello'], 'notes.txt', { type: 'text/plain' }), + ]); + }); + + const uploadBody = mockMutate.mock.calls[0][0] as FormData; + const fileId = uploadBody.get('file_id') as string; + + act(() => { + mockUploadOptions.onSuccess?.( + { + temp_file_id: 'an-id-the-client-never-sent', + file_id: 'saved-file-id', + filepath: '/files/notes.txt', + type: 'text/plain', + filename: 'notes.txt', + source: 'local', + embedded: false, + }, + uploadBody, + ); + }); + act(() => { + jest.runAllTimers(); + }); + jest.useRealTimers(); + + /** Removal deletes by the value's own ids, so a temporary id that is not the + * map key leaves a chip the user cannot clear. */ + const [, completion] = mockUpdateFileById.mock.calls.at(-1) as [string, { progress: number }]; + expect(completion).toMatchObject({ progress: 1, temp_file_id: fileId }); + consoleLog.mockRestore(); + }); + + it('releases the upload reservation when a decode fails', async () => { + /** A stable setter so both batches share one upload scope, and a file map that + * never observes the file — the render that would release the reservation + * cannot happen once the failed decode has removed it. */ + const sharedState = { + files: new Map(), + setFiles: jest.fn(), + setFilesLoading: mockSetFilesLoading, + }; + const { useFileHandlingNoChatContext } = await import('../useFileHandling'); + const { result, rerender } = renderHook(() => + useFileHandlingNoChatContext(undefined, sharedState), + ); + const pick = () => new File(['broken'], 'photo.png', { type: 'image/png' }); + + mockImageDecodes = false; + await act(async () => { + await result.current.handleFiles([pick()]); + }); + await act(async () => { + await Promise.resolve(); + }); + + rerender(); + mockValidateFileDuplicates.mockClear(); + mockImageDecodes = true; + await act(async () => { + await result.current.handleFiles([pick()]); + }); + + /** A reservation the failed decode left behind is merged into the next batch's + * validation, so re-picking the same file reads as a duplicate and its size + * keeps counting against the composer's limits. */ + const [{ files: validatedAgainst }] = mockValidateFileDuplicates.mock.calls[0]; + expect(validatedAgainst.size).toBe(0); + expect(mockMutate).toHaveBeenCalledTimes(1); + }); + + it('completes the attachment against the id the request was sent with', async () => { + const consoleLog = jest.spyOn(console, 'log').mockImplementation(() => undefined); + const useFileHandling = await loadHook(); + const { result } = renderHook(() => useFileHandling()); + + await act(async () => { + await result.current.handleFiles([ + new File(['hello'], 'notes.txt', { type: 'text/plain' }), + ]); + }); + + const uploadBody = mockMutate.mock.calls[0][0] as FormData; + const fileId = uploadBody.get('file_id') as string; + + act(() => { + mockUploadOptions.onSuccess?.( + { + temp_file_id: 'an-id-the-client-never-sent', + file_id: 'saved-file-id', + filepath: '/files/notes.txt', + type: 'text/plain', + filename: 'notes.txt', + source: 'local', + embedded: false, + }, + uploadBody, + ); + }); + + const updatedIds = mockUpdateFileById.mock.calls.map(([id]) => id); + expect(updatedIds).toContain(fileId); + expect(updatedIds).not.toContain('an-id-the-client-never-sent'); + consoleLog.mockRestore(); + }); + }); }); diff --git a/client/src/hooks/Files/useFileHandling.ts b/client/src/hooks/Files/useFileHandling.ts index 8ea2ef3f79..1eac7e627c 100644 --- a/client/src/hooks/Files/useFileHandling.ts +++ b/client/src/hooks/Files/useFileHandling.ts @@ -214,16 +214,23 @@ const useFileHandlingCore = (params: UseFileHandling | undefined, fileState: Fil const uploadFile = useUploadFileMutation( { - onSuccess: (data) => { - takeUploadRecovery(data.temp_file_id)?.onSuccess?.(data.temp_file_id); - clearUploadTimer(data.temp_file_id); + onSuccess: (data, body) => { + /** Every client-side handle for this upload — the file map key, the delayed + * toast timer, the recovery callbacks — is the id the request was sent with. + * `temp_file_id` is the server's echo of it, so trusting the echo turns any + * mismatch into a completion update applied to a key that does not exist: + * the attachment stays below `progress: 1` and the send button never + * re-enables. Reconcile against the id we own. */ + const fileId = (body.get('file_id') as string | null) ?? data.temp_file_id; + takeUploadRecovery(fileId)?.onSuccess?.(fileId); + clearUploadTimer(fileId); console.log('upload success', data); if (agent_id) { queryClient.refetchQueries([QueryKeys.agent, agent_id]); return; } updateFileById( - data.temp_file_id, + fileId, { progress: 0.9, filepath: data.filepath, @@ -232,17 +239,22 @@ const useFileHandlingCore = (params: UseFileHandling | undefined, fileState: Fil ); setTimeout(() => { - const cachedBlob = getCachedPreview(data.temp_file_id); - if (cachedBlob && data.file_id !== data.temp_file_id) { + const cachedBlob = getCachedPreview(fileId); + if (cachedBlob && data.file_id !== fileId) { cachePreview(data.file_id, cachedBlob); - removePreviewEntry(data.temp_file_id); + removePreviewEntry(fileId); } updateFileById( - data.temp_file_id, + fileId, { progress: 1, file_id: data.file_id, - temp_file_id: data.temp_file_id, + /** The stored temporary id has to stay the one this entry is keyed + * by: removal reads `file_id` and `temp_file_id` off the value and + * deletes those keys, and the draft restore correlates the cached + * record by the key it saved. Keeping the server's echo here would + * leave a chip that Remove deletes server-side but cannot clear. */ + temp_file_id: fileId, filepath: data.filepath, type: data.type, height: data.height, @@ -389,15 +401,35 @@ const useFileHandlingCore = (params: UseFileHandling | undefined, fileState: Fil ) => { const img = new Image(); img.onload = async () => { - extendedFile.width = img.width; - extendedFile.height = img.height; - extendedFile = { + const measuredFile: ExtendedFile = { ...extendedFile, + width: img.width, + height: img.height, progress: 0.6, }; - replaceFile(extendedFile); + replaceFile(measuredFile); - await startUpload(extendedFile, uploadLifecycle); + await startUpload(measuredFile, uploadLifecycle); + }; + /** The upload only starts once the browser has decoded the image, so a decode + * it refuses (unsupported codec, truncated bytes, a revoked object URL) would + * otherwise strand the attachment below `progress: 1` — which reads as "still + * uploading" and keeps the composer's send button disabled for the rest of the + * session, with nothing to click and no error to explain it. Drop the file and + * say so instead. */ + img.onerror = () => { + clearUploadTimer(extendedFile.file_id); + takeUploadRecovery(extendedFile.file_id)?.onError?.(extendedFile.file_id); + deleteFileById(extendedFile.file_id); + /** Reservations are released by the render that observes the file in the + * shared state, which a decode failing before that render never reaches — + * and once the file is gone no later render can either. A leaked one is + * merged into every subsequent batch's validation, so re-picking the same + * file reads as a duplicate and its size keeps counting against the limit. */ + uploadScope.recent.delete(extendedFile.file_id); + removePreviewEntry(extendedFile.file_id); + URL.revokeObjectURL(preview); + setError('com_error_files_process'); }; img.src = preview; }; diff --git a/client/src/hooks/Input/useAutoSave.spec.ts b/client/src/hooks/Input/useAutoSave.spec.ts index 5293207848..8331f9b54f 100644 --- a/client/src/hooks/Input/useAutoSave.spec.ts +++ b/client/src/hooks/Input/useAutoSave.spec.ts @@ -529,3 +529,131 @@ describe('useAutoSave — side-by-side pending drafts', () => { expect(getFilesDraft(`${Constants.NEW_CONVO}:1`)).toEqual({ fileIds: [], pendingPastes: {} }); }); }); + +describe('useAutoSave — file cache updates', () => { + const liveAttachment = { + file_id: 'client-temp-id', + type: 'image/png', + size: 2048, + progress: 0.9, + preview: 'blob:local-preview', + tool_resource: 'file_search', + file: new File(['bytes'], 'cat.png', { type: 'image/png' }), + }; + const persistedRecord = { + file_id: 'server-file-id', + temp_file_id: 'client-temp-id', + filename: 'cat.png', + filepath: '/images/cat.png', + type: 'image/png', + bytes: 2048, + object: 'file', + usage: 0, + user: 'user-1', + embedded: false, + }; + + const applySetFiles = (setFiles: jest.Mock, current: Map) => + setFiles.mock.calls.reduce( + (files, [update]) => (typeof update === 'function' ? update(files) : update), + current, + ) as Map>; + + /** + * The file cache is rewritten on every upload and on every attachment an agent + * emits mid-run, and this hook restores from it. An empty draft there means the + * draft write has not caught up — not that the composer is empty — so clearing + * would drop an attachment the user just added (and, with no text typed, leave + * them with nothing submittable). + */ + it('leaves live attachments alone when the file cache changes with no saved draft', () => { + const setFiles = jest.fn(); + const files = new Map([['client-temp-id', liveAttachment]]); + + const { rerender } = renderHook( + ({ fileList }: { fileList: unknown[] }) => { + (useGetFiles as jest.Mock).mockReturnValue({ data: fileList }); + return useAutoSave({ + conversationId: 'convo-1', + textAreaRef: makeTextAreaRef(), + files, + setFiles, + }); + }, + { initialProps: { fileList: [] as unknown[] } }, + ); + + setFiles.mockClear(); + /** The draft is gone the moment storage refuses or evicts the write — another + * tab clearing it, a quota failure, private browsing. The attachment the user + * just added is still in the composer either way. */ + localStorage.clear(); + act(() => { + rerender({ fileList: [persistedRecord] }); + }); + + expect(applySetFiles(setFiles, files).size).toBe(1); + }); + + /** + * The restore also lands on entries the composer still owns, so it has to layer + * the persisted record over them rather than replace them: the blob preview the + * chip renders from and the tool resource the upload was staged under exist only + * locally, and `attached` decides whether removing the chip deletes the file. + */ + it('layers the persisted record over a live attachment instead of replacing it', () => { + const setFiles = jest.fn(); + const files = new Map([['client-temp-id', liveAttachment]]); + setFilesDraft('convo-1', { fileIds: ['client-temp-id'], pendingPastes: {} }); + + const { rerender } = renderHook( + ({ fileList }: { fileList: unknown[] }) => { + (useGetFiles as jest.Mock).mockReturnValue({ data: fileList }); + return useAutoSave({ + conversationId: 'convo-1', + textAreaRef: makeTextAreaRef(), + files, + setFiles, + }); + }, + { initialProps: { fileList: [] as unknown[] } }, + ); + + /** Past the mount swap, which clears the composer itself before restoring. */ + setFiles.mockClear(); + act(() => { + rerender({ fileList: [persistedRecord] }); + }); + + const restored = applySetFiles(setFiles, files).get('client-temp-id'); + expect(restored).toMatchObject({ + file_id: 'server-file-id', + filepath: '/images/cat.png', + progress: 1, + preview: 'blob:local-preview', + tool_resource: 'file_search', + attached: false, + }); + expect(restored?.file).toBeInstanceOf(File); + }); + + it('marks a file restored from a draft alone as attached', () => { + const setFiles = jest.fn(); + setFilesDraft('convo-1', { fileIds: ['client-temp-id'], pendingPastes: {} }); + + renderHook(() => { + (useGetFiles as jest.Mock).mockReturnValue({ data: [persistedRecord] }); + return useAutoSave({ + conversationId: 'convo-1', + textAreaRef: makeTextAreaRef(), + files: new Map(), + setFiles, + }); + }); + + expect(applySetFiles(setFiles, new Map()).get('client-temp-id')).toMatchObject({ + attached: true, + progress: 1, + }); + }); +}); diff --git a/client/src/hooks/Input/useAutoSave.ts b/client/src/hooks/Input/useAutoSave.ts index 15687b686a..ee9ce33ca2 100644 --- a/client/src/hooks/Input/useAutoSave.ts +++ b/client/src/hooks/Input/useAutoSave.ts @@ -65,11 +65,12 @@ export const useAutoSave = ({ (id: string): PendingTextAttachmentDraft[] => { const filesDraft = getFilesDraft(id); - if (filesDraft.fileIds.length === 0) { - setFiles(new Map()); - return []; - } - if (fileList == null) { + /** Restoring adds what the draft holds; it never clears. The conversation + * swap below owns that, and does it explicitly before calling here — while + * the file-cache path runs on every `QueryKeys.files` write (an upload + * landing, an SSE attachment during a run), where an empty draft means the + * write has not caught up yet, not that the user has no attachments. */ + if (filesDraft.fileIds.length === 0 || fileList == null) { return []; } @@ -95,10 +96,20 @@ export const useAutoSave = ({ delete pendingPastes[fileId]; setFiles((currentFiles) => { const updatedFiles = new Map(currentFiles); + /** The same entry may still be live in the composer — this path also + * runs when the upload that created it writes the file cache. Layer + * the persisted record over it instead of replacing it: the local + * `File`, the blob preview the chip renders from, and the tool + * resource the upload was staged under exist nowhere on the server + * record, and `attached` decides whether removing the chip also + * deletes the file, which a composer-owned upload still needs. */ + const live = updatedFiles.get(fileIdToRecover); updatedFiles.set(fileIdToRecover, { + ...live, ...fileToRecover, + preview: live?.preview ?? fileToRecover.preview, progress: 1, - attached: true, + attached: live ? (live.attached ?? false) : true, size: fileToRecover.bytes, }); return updatedFiles;