mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-21 17:44:11 +00:00
🧷 fix: Flush Pending File Deletion on Unmount (#14293)
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
Removing the last file from an agent's Context panel fired no request and the file reappeared on reload. `FileContext` mounts `FileRow` only while `fileCount > 0`, and `useFileDeletion` lives inside `FileRow` behind a 1s debounce whose unmount cleanup called `debouncedDelete.cancel()`. Deleting the last file drops the count to 0, unmounting `FileRow` and cancelling the delete the user had already confirmed. Removing a non-last file kept the row mounted and worked, which made the failure look erratic. The same cancel also dropped deletes in any panel closed within the debounce window. Flush the pending batch on unmount instead of cancelling it.
This commit is contained in:
parent
abcbc40b8e
commit
9c7547db96
2 changed files with 140 additions and 4 deletions
135
client/src/hooks/Files/__tests__/useFileDeletion.spec.tsx
Normal file
135
client/src/hooks/Files/__tests__/useFileDeletion.spec.tsx
Normal file
|
|
@ -0,0 +1,135 @@
|
|||
import React, { useState } from 'react';
|
||||
import { FileSources, EToolResources } from 'librechat-data-provider';
|
||||
import { render, screen, act, renderHook } from '@testing-library/react';
|
||||
import type { ExtendedFile } from '~/common';
|
||||
import FileRow from '~/components/Chat/Input/Files/FileRow';
|
||||
import useFileDeletion from '../useFileDeletion';
|
||||
|
||||
const mockMutateAsync = jest.fn();
|
||||
|
||||
jest.mock('~/data-provider', () => ({
|
||||
useDeleteFilesMutation: () => ({ mutateAsync: mockMutateAsync }),
|
||||
}));
|
||||
|
||||
jest.mock('@librechat/client', () => ({
|
||||
useToastContext: () => ({ showToast: jest.fn() }),
|
||||
}));
|
||||
|
||||
jest.mock('~/hooks', () => ({
|
||||
useLocalize: () => (key: string) => key,
|
||||
}));
|
||||
|
||||
jest.mock('~/utils', () => ({
|
||||
...jest.requireActual('~/utils'),
|
||||
logger: { log: jest.fn() },
|
||||
getCachedPreview: () => undefined,
|
||||
deletePreview: jest.fn(),
|
||||
}));
|
||||
|
||||
jest.mock('../useSetFilesToDelete', () => ({
|
||||
__esModule: true,
|
||||
default: () => jest.fn(),
|
||||
}));
|
||||
|
||||
jest.mock('~/components/Chat/Input/Files/FileContainer', () => {
|
||||
return function MockFileContainer({
|
||||
file,
|
||||
onDelete,
|
||||
}: {
|
||||
file: ExtendedFile;
|
||||
onDelete: () => void;
|
||||
}) {
|
||||
return <button data-testid={`del-${file.file_id}`} onClick={onDelete} />;
|
||||
};
|
||||
});
|
||||
|
||||
jest.mock('~/components/Chat/Input/Files/Image', () => {
|
||||
return function MockImage() {
|
||||
return null;
|
||||
};
|
||||
});
|
||||
|
||||
/** Mirrors the shape `utils/forms.tsx` builds for agent Context/File Search panels */
|
||||
const makeFile = (file_id: string): ExtendedFile =>
|
||||
({
|
||||
file_id,
|
||||
type: 'application/pdf',
|
||||
filepath: `/uploads/${file_id}.pdf`,
|
||||
filename: `${file_id}.pdf`,
|
||||
size: 1024,
|
||||
progress: 1,
|
||||
source: FileSources.local,
|
||||
}) as ExtendedFile;
|
||||
|
||||
/** Mirrors FileContext.tsx, which mounts FileRow only while `fileCount > 0` */
|
||||
function ConditionalPanel({ initial }: { initial: ExtendedFile[] }) {
|
||||
const [files, setFiles] = useState(new Map(initial.map((f) => [f.file_id, f])));
|
||||
return (
|
||||
<>
|
||||
{files.size > 0 && (
|
||||
<FileRow
|
||||
files={files}
|
||||
setFiles={setFiles}
|
||||
agent_id="agent-123"
|
||||
tool_resource={EToolResources.context}
|
||||
/>
|
||||
)}
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
describe('useFileDeletion', () => {
|
||||
beforeEach(() => {
|
||||
jest.useFakeTimers();
|
||||
mockMutateAsync.mockClear();
|
||||
});
|
||||
afterEach(() => jest.useRealTimers());
|
||||
|
||||
const clickDelete = (file_id: string) => {
|
||||
act(() => {
|
||||
screen.getByTestId(`del-${file_id}`).click();
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(3000);
|
||||
});
|
||||
};
|
||||
|
||||
it('sends the unlink request when removing the last file from an agent panel', () => {
|
||||
render(<ConditionalPanel initial={[makeFile('only-file')]} />);
|
||||
|
||||
clickDelete('only-file');
|
||||
|
||||
expect(mockMutateAsync).toHaveBeenCalledTimes(1);
|
||||
expect(mockMutateAsync).toHaveBeenCalledWith({
|
||||
files: [expect.objectContaining({ file_id: 'only-file' })],
|
||||
agent_id: 'agent-123',
|
||||
tool_resource: EToolResources.context,
|
||||
});
|
||||
});
|
||||
|
||||
it('sends the unlink request when removing a non-last file from an agent panel', () => {
|
||||
render(<ConditionalPanel initial={[makeFile('file-a'), makeFile('file-b')]} />);
|
||||
|
||||
clickDelete('file-a');
|
||||
|
||||
expect(mockMutateAsync).toHaveBeenCalledTimes(1);
|
||||
expect(mockMutateAsync).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ agent_id: 'agent-123', tool_resource: EToolResources.context }),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not delete a file that was attached from existing storage', () => {
|
||||
const { result } = renderHook(() => useFileDeletion({ mutateAsync: mockMutateAsync }));
|
||||
|
||||
act(() => {
|
||||
result.current.deleteFile({
|
||||
file: { ...makeFile('stored-file'), attached: true } as ExtendedFile,
|
||||
});
|
||||
});
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(3000);
|
||||
});
|
||||
|
||||
expect(mockMutateAsync).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
|
@ -1,8 +1,8 @@
|
|||
import { useCallback, useState, useEffect } from 'react';
|
||||
import debounce from 'lodash/debounce';
|
||||
import { FileSources, EToolResources, removeNullishValues } from 'librechat-data-provider';
|
||||
import { useCallback, useState, useEffect } from 'react';
|
||||
import type * as t from 'librechat-data-provider';
|
||||
import type { UseMutateAsyncFunction } from '@tanstack/react-query';
|
||||
import type * as t from 'librechat-data-provider';
|
||||
import type { ExtendedFile, GenericSetter } from '~/common';
|
||||
import useSetFilesToDelete from './useSetFilesToDelete';
|
||||
import { deletePreview } from '~/utils';
|
||||
|
|
@ -51,8 +51,9 @@ const useFileDeletion = ({
|
|||
const debouncedDelete = useCallback(debounce(executeBatchDelete, 1000), []);
|
||||
|
||||
useEffect(() => {
|
||||
// Cleanup function for debouncedDelete when component unmounts or before re-render
|
||||
return () => debouncedDelete.cancel();
|
||||
/** Flush, don't cancel: unmount is a normal outcome of removing the last file,
|
||||
* and a cancelled batch silently drops a delete the user already confirmed. */
|
||||
return () => debouncedDelete.flush();
|
||||
}, [debouncedDelete]);
|
||||
|
||||
const deleteFile = useCallback(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue