mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-21 09:32:24 +00:00
📎 fix: Never Let a Stalled Attachment Disable the Composer (#15013)
* 🖼️ fix: Keep Composer Send Enabled When an Attachment Stalls The composer's send button is gated on `hasIncompleteFiles(files)`, so any attachment that can never reach `progress: 1` reads as "still uploading" and disables send for the rest of the session — draft text intact, no error, no way out but removing the chip or reloading. Two paths could park an attachment there: - `loadImage` starts the upload from `img.onload` and had no `onerror`, so an image the browser refuses to decode (unsupported codec, truncated bytes, a revoked object URL) never uploaded at all and stranded the file at `progress: 0.2`. Drop the file and surface the error instead. - Upload completion reconciled against `temp_file_id`, the server's echo of the id the request was sent with, while every client-side handle for that upload — file map key, delayed-toast timer, recovery callbacks — is keyed by the id the client owns. A mismatch applied the completion update to a key that does not exist, leaving the attachment at `progress: 0.9`. Covered by unit regressions in the file-handling suite and a composer-level spec that drives a real upload through `ChatForm`, plus a render-bound guard on typing (react-scan measures one ChatForm render per keystroke in a browser; the guard fails on a multiplier). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cq3tev2rPbc2pWyVJwnh6u * 🧹 fix: Stop the Draft Restore From Clobbering Live Composer Attachments `restoreFiles` runs on every `QueryKeys.files` write — an upload landing, an SSE attachment mid-run — not just on a conversation swap, and it was written as if the draft were always the whole truth: - An empty draft cleared the composer outright. On the swap path that is redundant (the effect already clears explicitly one line earlier); on the cache path an empty draft only means the draft write has not caught up, so clearing there discards an attachment the user just added — and with no text typed, the send button has nothing left to submit. Restoring now only adds. - A match replaced the composer's entry with the persisted record, dropping the local `File`, the blob preview the chip renders from (`FileRow` falls back to refetching `filepath`), and the tool resource the upload was staged under, and stamping `attached: true` so removing a chip the composer still owns leaves the file orphaned server-side. It now layers the record over the live entry and leaves `attached` to files actually adopted from a draft. Confirmed against a real browser run: the entry is at `progress: 0.9` when this restore fires, so it — not the upload's own completion — is what was re-enabling send. react-scan render counts are unchanged (typing 20 keystrokes: 111 renders, ChatForm=20; attaching an image: 1373, FileRow=6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cq3tev2rPbc2pWyVJwnh6u * 🔗 fix: Keep an Attachment's Stored Temporary Id Equal to Its Map Key Two follow-ups from review on the upload reconciliation. Completion stored the server's `temp_file_id` echo in the entry's value while keying the map by the id the request was sent with. `useFileDeletion` deletes map entries by the value's own `file_id` and `temp_file_id`, so where the two disagreed — the exact case the reconciliation exists to tolerate — Remove would delete the file server-side and leave the chip behind, and the draft restore could not correlate its saved key with the cached record. Store the request id. A refused image decode also left its `uploadScope.recent` reservation behind: 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 deleted no later render can either. The ghost is merged into every later batch's validation, so re-picking the same file reads as a duplicate and its size keeps counting against the composer's limits. Both covered; both new guards fail without their fix. Also sorts the composer spec's imports, which the static-checks import-order gate flagged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cq3tev2rPbc2pWyVJwnh6u * 🧷 fix: Normalize an Upload's Temporary Id at the Cache Boundary The composer keys its file map — and the draft it saves — by the `file_id` the upload request was sent with; `temp_file_id` is only the server's echo of that id. The previous commit reconciled the composer's own entry against the request id but left the record the mutation inserts into `QueryKeys.files` carrying the raw echo, and `restoreFiles` can only correlate a saved draft id by matching a cached record's `file_id` or `temp_file_id`. Where the echo disagreed the draft matched neither, so the attachment was silently dropped on the next conversation switch or reload — the same class of loss, one layer further out. Normalize once where the response enters client state, and hand the normalized record to the mutation's callers, so the cache, the composer entry and the draft all agree on one id. An agreeing response is passed through untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cq3tev2rPbc2pWyVJwnh6u --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Danny Avila
GitHub
parent
c8953b8f32
commit
2f50e38217
7 changed files with 752 additions and 49 deletions
|
|
@ -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<ChatFormValues>({ 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<typeof ChatContext>,
|
||||
[files, setFiles, isSubmitting],
|
||||
);
|
||||
|
||||
return (
|
||||
<ChatFormProvider {...methods}>
|
||||
<ChatContext.Provider value={chatHelpers}>
|
||||
<Profiler id="composer" onRender={() => (commits += 1)}>
|
||||
<ChatForm index={0} />
|
||||
</Profiler>
|
||||
</ChatContext.Provider>
|
||||
</ChatFormProvider>
|
||||
);
|
||||
}
|
||||
|
||||
function renderComposer() {
|
||||
const queryClient = new QueryClient({
|
||||
defaultOptions: { queries: { retry: false }, mutations: { retry: false } },
|
||||
});
|
||||
queryClient.setQueryData([QueryKeys.fileConfig], {});
|
||||
queryClient.setQueryData<TFile[]>([QueryKeys.files], []);
|
||||
queryClient.setQueryData([QueryKeys.endpoints], { [EModelEndpoint.openAI]: { order: 0 } });
|
||||
|
||||
return render(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<RecoilRoot>
|
||||
<Router>
|
||||
<AuthContextProvider authConfig={{ loginRedirect: '', test: true }}>
|
||||
<DndProvider backend={HTML5Backend}>
|
||||
<Harness />
|
||||
</DndProvider>
|
||||
</AuthContextProvider>
|
||||
</Router>
|
||||
</RecoilRoot>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
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);
|
||||
});
|
||||
|
|
@ -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 }) => (
|
||||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
|
||||
);
|
||||
|
||||
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<TFile[]>([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<TFile[]>([QueryKeys.files]) ?? [];
|
||||
expect(cached).toBe(onSuccess.mock.calls[0][0]);
|
||||
});
|
||||
});
|
||||
|
|
@ -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<t.TFile[] | undefined>([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);
|
||||
},
|
||||
});
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>) =>
|
||||
setFiles.mock.calls.reduce(
|
||||
(files, [update]) => (typeof update === 'function' ? update(files) : update),
|
||||
current,
|
||||
) as Map<string, Record<string, unknown>>;
|
||||
|
||||
/**
|
||||
* 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,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue