From 0b071c06f68828ec542ddf8439947ccfb96dc233 Mon Sep 17 00:00:00 2001 From: Sebastien Bruel <93573440+sbruel@users.noreply.github.com> Date: Thu, 7 Aug 2025 15:12:05 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=A5=9E=20refactor:=20Duplicate=20Agent=20?= =?UTF-8?q?Versions=20as=20Informational=20Instead=20of=20Errors=20(#8881)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix error when updating an agent with no changes * Add tests * Revert translation file changes --- api/models/Agent.js | 15 +- api/models/Agent.spec.js | 93 ++--- api/server/controllers/agents/v1.js | 3 + api/server/controllers/agents/v1.spec.js | 22 + .../SidePanel/Agents/AgentPanel.test.tsx | 380 ++++++++++++++++++ .../SidePanel/Agents/AgentPanel.tsx | 64 +-- client/src/data-provider/Agents/mutations.ts | 9 +- client/src/locales/en/translation.json | 1 - packages/data-provider/src/types/mutations.ts | 7 +- 9 files changed, 474 insertions(+), 120 deletions(-) create mode 100644 client/src/components/SidePanel/Agents/AgentPanel.test.tsx diff --git a/api/models/Agent.js b/api/models/Agent.js index 7cb32e6cb4..a853576711 100644 --- a/api/models/Agent.js +++ b/api/models/Agent.js @@ -316,17 +316,10 @@ const updateAgent = async (searchParameter, updateData, options = {}) => { if (shouldCreateVersion) { const duplicateVersion = isDuplicateVersion(updateData, versionData, versions, actionsHash); if (duplicateVersion && !forceVersion) { - const error = new Error( - 'Duplicate version: This would create a version identical to an existing one', - ); - error.statusCode = 409; - error.details = { - duplicateVersion, - versionIndex: versions.findIndex( - (v) => JSON.stringify(duplicateVersion) === JSON.stringify(v), - ), - }; - throw error; + // No changes detected, return the current agent without creating a new version + const agentObj = currentAgent.toObject(); + agentObj.version = versions.length; + return agentObj; } } diff --git a/api/models/Agent.spec.js b/api/models/Agent.spec.js index 8953ae0482..dcd46bc6dd 100644 --- a/api/models/Agent.spec.js +++ b/api/models/Agent.spec.js @@ -879,45 +879,31 @@ describe('models/Agent', () => { expect(emptyParamsAgent.model_parameters).toEqual({}); }); - test('should detect duplicate versions and reject updates', async () => { - const originalConsoleError = console.error; - console.error = jest.fn(); + test('should not create new version for duplicate updates', async () => { + const authorId = new mongoose.Types.ObjectId(); + const testCases = generateVersionTestCases(); - try { - const authorId = new mongoose.Types.ObjectId(); - const testCases = generateVersionTestCases(); + for (const testCase of testCases) { + const testAgentId = `agent_${uuidv4()}`; - for (const testCase of testCases) { - const testAgentId = `agent_${uuidv4()}`; + await createAgent({ + id: testAgentId, + provider: 'test', + model: 'test-model', + author: authorId, + ...testCase.initial, + }); - await createAgent({ - id: testAgentId, - provider: 'test', - model: 'test-model', - author: authorId, - ...testCase.initial, - }); + const updatedAgent = await updateAgent({ id: testAgentId }, testCase.update); + expect(updatedAgent.versions).toHaveLength(2); // No new version created - await updateAgent({ id: testAgentId }, testCase.update); + // Update with duplicate data should succeed but not create a new version + const duplicateUpdate = await updateAgent({ id: testAgentId }, testCase.duplicate); - let error; - try { - await updateAgent({ id: testAgentId }, testCase.duplicate); - } catch (e) { - error = e; - } + expect(duplicateUpdate.versions).toHaveLength(2); // No new version created - expect(error).toBeDefined(); - expect(error.message).toContain('Duplicate version'); - expect(error.statusCode).toBe(409); - expect(error.details).toBeDefined(); - expect(error.details.duplicateVersion).toBeDefined(); - - const agent = await getAgent({ id: testAgentId }); - expect(agent.versions).toHaveLength(2); - } - } finally { - console.error = originalConsoleError; + const agent = await getAgent({ id: testAgentId }); + expect(agent.versions).toHaveLength(2); } }); @@ -1093,20 +1079,13 @@ describe('models/Agent', () => { expect(secondUpdate.versions).toHaveLength(3); // Update without forceVersion and no changes should not create a version - let error; - try { - await updateAgent( - { id: agentId }, - { tools: ['listEvents_action_test.com', 'createEvent_action_test.com'] }, - { updatingUserId: authorId.toString(), forceVersion: false }, - ); - } catch (e) { - error = e; - } + const duplicateUpdate = await updateAgent( + { id: agentId }, + { tools: ['listEvents_action_test.com', 'createEvent_action_test.com'] }, + { updatingUserId: authorId.toString(), forceVersion: false }, + ); - expect(error).toBeDefined(); - expect(error.message).toContain('Duplicate version'); - expect(error.statusCode).toBe(409); + expect(duplicateUpdate.versions).toHaveLength(3); // No new version created }); test('should handle isDuplicateVersion with arrays containing null/undefined values', async () => { @@ -2400,11 +2379,18 @@ describe('models/Agent', () => { agent_ids: ['agent1', 'agent2'], }); - await updateAgent({ id: agentId }, { agent_ids: ['agent1', 'agent2', 'agent3'] }); + const updatedAgent = await updateAgent( + { id: agentId }, + { agent_ids: ['agent1', 'agent2', 'agent3'] }, + ); + expect(updatedAgent.versions).toHaveLength(2); - await expect( - updateAgent({ id: agentId }, { agent_ids: ['agent1', 'agent2', 'agent3'] }), - ).rejects.toThrow('Duplicate version'); + // Update with same agent_ids should succeed but not create a new version + const duplicateUpdate = await updateAgent( + { id: agentId }, + { agent_ids: ['agent1', 'agent2', 'agent3'] }, + ); + expect(duplicateUpdate.versions).toHaveLength(2); // No new version created }); test('should handle agent_ids field alongside other fields', async () => { @@ -2543,9 +2529,10 @@ describe('models/Agent', () => { expect(updated.versions).toHaveLength(2); expect(updated.agent_ids).toEqual([]); - await expect(updateAgent({ id: agentId }, { agent_ids: [] })).rejects.toThrow( - 'Duplicate version', - ); + // Update with same empty agent_ids should succeed but not create a new version + const duplicateUpdate = await updateAgent({ id: agentId }, { agent_ids: [] }); + expect(duplicateUpdate.versions).toHaveLength(2); // No new version created + expect(duplicateUpdate.agent_ids).toEqual([]); }); test('should handle agent without agent_ids field', async () => { diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index c3c6167605..32d682d6d1 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -194,6 +194,9 @@ const updateAgentHandler = async (req, res) => { }); } + // Add version count to the response + updatedAgent.version = updatedAgent.versions ? updatedAgent.versions.length : 0; + if (updatedAgent.author) { updatedAgent.author = updatedAgent.author.toString(); } diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index 5ac2645c04..0574dde4eb 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -498,6 +498,28 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(mockRes.json).toHaveBeenCalledWith({ error: 'Agent not found' }); }); + test('should include version field in update response', async () => { + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Updated with Version Check', + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const updatedAgent = mockRes.json.mock.calls[0][0]; + + // Verify version field is included and is a number + expect(updatedAgent).toHaveProperty('version'); + expect(typeof updatedAgent.version).toBe('number'); + expect(updatedAgent.version).toBeGreaterThanOrEqual(1); + + // Verify in database + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(updatedAgent.version).toBe(agentInDb.versions.length); + }); + test('should handle validation errors properly', async () => { mockReq.user.id = existingAgentAuthorId.toString(); mockReq.params.id = existingAgentId; diff --git a/client/src/components/SidePanel/Agents/AgentPanel.test.tsx b/client/src/components/SidePanel/Agents/AgentPanel.test.tsx new file mode 100644 index 0000000000..c8129abe04 --- /dev/null +++ b/client/src/components/SidePanel/Agents/AgentPanel.test.tsx @@ -0,0 +1,380 @@ +/** + * @jest-environment jsdom + */ +import * as React from 'react'; +import { describe, it, expect, beforeEach, jest } from '@jest/globals'; +import { render, waitFor, fireEvent } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type { Agent } from 'librechat-data-provider'; + +// Mock toast context - define this after all mocks +let mockShowToast: jest.Mock; + +// Mock notification severity enum before other imports +jest.mock('~/common/types', () => ({ + NotificationSeverity: { + SUCCESS: 'success', + ERROR: 'error', + INFO: 'info', + WARNING: 'warning', + }, +})); + +// Mock store to prevent import errors +jest.mock('~/store/toast', () => ({ + default: () => ({ + showToast: jest.fn(), + }), +})); + +jest.mock('~/store', () => {}); + +// Mock the data service to control network responses +jest.mock('librechat-data-provider', () => { + const actualModule = jest.requireActual('librechat-data-provider') as any; + return { + ...actualModule, + dataService: { + updateAgent: jest.fn(), + }, + Tools: { + execute_code: 'execute_code', + file_search: 'file_search', + web_search: 'web_search', + }, + Constants: { + EPHEMERAL_AGENT_ID: 'ephemeral', + }, + SystemRoles: { + ADMIN: 'ADMIN', + }, + EModelEndpoint: { + agents: 'agents', + chatGPTBrowser: 'chatGPTBrowser', + gptPlugins: 'gptPlugins', + }, + isAssistantsEndpoint: jest.fn(() => false), + }; +}); + +jest.mock('@librechat/client', () => ({ + Button: ({ children, onClick, ...props }: any) => ( + + ), + useToastContext: () => ({ + get showToast() { + return mockShowToast || jest.fn(); + }, + }), +})); + +// Mock other dependencies +jest.mock('librechat-data-provider/react-query', () => ({ + useGetModelsQuery: () => ({ data: {} }), +})); + +jest.mock('~/utils', () => ({ + createProviderOption: jest.fn((provider: string) => ({ value: provider, label: provider })), + getDefaultAgentFormValues: jest.fn(() => ({ + id: '', + name: '', + description: '', + model: '', + provider: '', + })), +})); + +jest.mock('~/hooks', () => ({ + useSelectAgent: () => ({ onSelect: jest.fn() }), + useLocalize: () => (key: string) => key, + useAuthContext: () => ({ user: { id: 'user-123', role: 'USER' } }), +})); + +jest.mock('~/Providers/AgentPanelContext', () => ({ + useAgentPanelContext: () => ({ + activePanel: 'builder', + agentsConfig: { allowedProviders: [] }, + setActivePanel: jest.fn(), + endpointsConfig: {}, + setCurrentAgentId: jest.fn(), + agent_id: 'agent-123', + }), +})); + +jest.mock('~/common', () => ({ + Panel: { + model: 'model', + builder: 'builder', + advanced: 'advanced', + }, +})); + +// Mock child components to simplify testing +jest.mock('./AgentPanelSkeleton', () => ({ + __esModule: true, + default: () =>
{`Loading...`}
, +})); + +jest.mock('./Advanced/AdvancedPanel', () => ({ + __esModule: true, + default: () =>
{`Advanced Panel`}
, +})); + +jest.mock('./AgentConfig', () => ({ + __esModule: true, + default: () =>
{`Agent Config`}
, +})); + +jest.mock('./AgentSelect', () => ({ + __esModule: true, + default: () =>
{`Agent Select`}
, +})); + +jest.mock('./ModelPanel', () => ({ + __esModule: true, + default: () =>
{`Model Panel`}
, +})); + +// Mock AgentFooter to provide a save button +jest.mock('./AgentFooter', () => ({ + __esModule: true, + default: () => ( + + ), +})); + +// Mock react-hook-form to capture form submission +let mockFormSubmitHandler: (() => void) | null = null; + +jest.mock('react-hook-form', () => { + const actual = jest.requireActual('react-hook-form') as any; + return { + ...actual, + useForm: () => { + const methods = actual.useForm({ + defaultValues: { + id: 'agent-123', + name: 'Test Agent', + description: 'Test description', + model: 'gpt-4', + provider: 'openai', + tools: [], + execute_code: false, + file_search: false, + web_search: false, + }, + }); + + return { + ...methods, + handleSubmit: (onSubmit: any) => (e?: any) => { + e?.preventDefault?.(); + mockFormSubmitHandler = () => onSubmit(methods.getValues()); + return mockFormSubmitHandler; + }, + }; + }, + FormProvider: ({ children }: any) => children, + useWatch: () => 'agent-123', + }; +}); + +// Import after mocks +import { dataService } from 'librechat-data-provider'; +import { useGetAgentByIdQuery } from '~/data-provider'; +import AgentPanel from './AgentPanel'; + +// Mock useGetAgentByIdQuery +jest.mock('~/data-provider', () => { + const actual = jest.requireActual('~/data-provider') as any; + return { + ...actual, + useGetAgentByIdQuery: jest.fn(), + useUpdateAgentMutation: actual.useUpdateAgentMutation, + }; +}); + +// Test wrapper with QueryClient +const createWrapper = () => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, + }); + + return ({ children }: { children: React.ReactNode }) => ( + {children} + ); +}; + +// Test helpers +const setupMocks = () => { + const mockUseGetAgentByIdQuery = useGetAgentByIdQuery as jest.MockedFunction< + typeof useGetAgentByIdQuery + >; + const mockUpdateAgent = dataService.updateAgent as jest.MockedFunction< + typeof dataService.updateAgent + >; + + return { mockUseGetAgentByIdQuery, mockUpdateAgent }; +}; + +const mockAgentQuery = ( + mockUseGetAgentByIdQuery: jest.MockedFunction, + agent: Partial, +) => { + mockUseGetAgentByIdQuery.mockReturnValue({ + data: { + id: 'agent-123', + author: 'user-123', + isCollaborative: false, + ...agent, + } as Agent, + isInitialLoading: false, + } as any); +}; + +const createMockAgent = (overrides: Partial = {}): Agent => + ({ + id: 'agent-123', + provider: 'openai', + model: 'gpt-4', + ...overrides, + }) as Agent; + +const renderAndSubmitForm = async () => { + const Wrapper = createWrapper(); + const { container, rerender } = render(, { wrapper: Wrapper }); + + const form = container.querySelector('form'); + expect(form).toBeTruthy(); + + fireEvent.submit(form!); + + if (mockFormSubmitHandler) { + mockFormSubmitHandler(); + } + + return { container, rerender, form }; +}; + +describe('AgentPanel - Update Agent Toast Messages', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockShowToast = jest.fn(); + mockFormSubmitHandler = null; + }); + + describe('AgentPanel', () => { + it('should show "no changes" toast when version does not change', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + // Mock the agent query with version 2 + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 2, + }); + + // Mock network response - same version + mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 2 })); + + await renderAndSubmitForm(); + + // Wait for the toast to be shown + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_ui_no_changes', + status: 'info', + }); + }); + }); + + it('should show "update success" toast when version changes', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + // Mock the agent query with version 2 + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 2, + }); + + // Mock network response - different version + mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 3 })); + + await renderAndSubmitForm(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_assistants_update_success Test Agent', + }); + }); + }); + + it('should show "update success" with default name when agent has no name', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + // Mock the agent query without name + mockAgentQuery(mockUseGetAgentByIdQuery, { + version: 1, + }); + + // Mock network response - no name + mockUpdateAgent.mockResolvedValue(createMockAgent({ version: 2 })); + + await renderAndSubmitForm(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_assistants_update_success com_ui_agent', + }); + }); + }); + + it('should show "update success" when agent query has no version (undefined)', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + // Mock the agent query with no version data + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + // No version property + }); + + mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 1 })); + + await renderAndSubmitForm(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_assistants_update_success Test Agent', + }); + }); + }); + + it('should show error toast on update failure', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + // Mock the agent query + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 1, + }); + + // Mock network error + mockUpdateAgent.mockRejectedValue(new Error('Update failed')); + + await renderAndSubmitForm(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_agents_update_error com_ui_error: Update failed', + status: 'error', + }); + }); + }); + }); +}); diff --git a/client/src/components/SidePanel/Agents/AgentPanel.tsx b/client/src/components/SidePanel/Agents/AgentPanel.tsx index 37a7d33d59..1f97ae09c2 100644 --- a/client/src/components/SidePanel/Agents/AgentPanel.tsx +++ b/client/src/components/SidePanel/Agents/AgentPanel.tsx @@ -1,5 +1,5 @@ import { Plus } from 'lucide-react'; -import React, { useMemo, useCallback } from 'react'; +import React, { useMemo, useCallback, useRef } from 'react'; import { Button, useToastContext } from '@librechat/client'; import { useWatch, useForm, FormProvider } from 'react-hook-form'; import { useGetModelsQuery } from 'librechat-data-provider/react-query'; @@ -54,6 +54,7 @@ export default function AgentPanel() { const { control, handleSubmit, reset } = methods; const agent_id = useWatch({ control, name: 'id' }); + const previousVersionRef = useRef(); const allowedProviders = useMemo( () => new Set(agentsConfig?.allowedProviders), @@ -77,50 +78,29 @@ export default function AgentPanel() { /* Mutations */ const update = useUpdateAgentMutation({ + onMutate: () => { + // Store the current version before mutation + previousVersionRef.current = agentQuery.data?.version; + }, onSuccess: (data) => { - showToast({ - message: `${localize('com_assistants_update_success')} ${ - data.name ?? localize('com_ui_agent') - }`, - }); + // Check if agent version is the same (no changes were made) + if (previousVersionRef.current !== undefined && data.version === previousVersionRef.current) { + showToast({ + message: localize('com_ui_no_changes'), + status: 'info', + }); + } else { + showToast({ + message: `${localize('com_assistants_update_success')} ${ + data.name ?? localize('com_ui_agent') + }`, + }); + } + // Clear the ref after use + previousVersionRef.current = undefined; }, onError: (err) => { - const error = err as Error & { - statusCode?: number; - details?: { duplicateVersion?: any; versionIndex?: number }; - response?: { status?: number; data?: any }; - }; - - const isDuplicateVersionError = - (error.statusCode === 409 && error.details?.duplicateVersion) || - (error.response?.status === 409 && error.response?.data?.details?.duplicateVersion); - - if (isDuplicateVersionError) { - let versionIndex: number | undefined = undefined; - - if (error.details?.versionIndex !== undefined) { - versionIndex = error.details.versionIndex; - } else if (error.response?.data?.details?.versionIndex !== undefined) { - versionIndex = error.response.data.details.versionIndex; - } - - if (versionIndex === undefined || versionIndex < 0) { - showToast({ - message: localize('com_agents_update_error'), - status: 'error', - duration: 5000, - }); - } else { - showToast({ - message: localize('com_ui_agent_version_duplicate', { versionIndex: versionIndex + 1 }), - status: 'error', - duration: 10000, - }); - } - - return; - } - + const error = err as Error; showToast({ message: `${localize('com_agents_update_error')}${ error.message ? ` ${localize('com_ui_error')}: ${error.message}` : '' diff --git a/client/src/data-provider/Agents/mutations.ts b/client/src/data-provider/Agents/mutations.ts index 8fd3a842a8..6e2935708d 100644 --- a/client/src/data-provider/Agents/mutations.ts +++ b/client/src/data-provider/Agents/mutations.ts @@ -43,11 +43,7 @@ export const useCreateAgentMutation = ( */ export const useUpdateAgentMutation = ( options?: t.UpdateAgentMutationOptions, -): UseMutationResult< - t.Agent, - t.DuplicateVersionError, - { agent_id: string; data: t.AgentUpdateParams } -> => { +): UseMutationResult => { const queryClient = useQueryClient(); return useMutation( ({ agent_id, data }: { agent_id: string; data: t.AgentUpdateParams }) => { @@ -59,8 +55,7 @@ export const useUpdateAgentMutation = ( { onMutate: (variables) => options?.onMutate?.(variables), onError: (error, variables, context) => { - const typedError = error as t.DuplicateVersionError; - return options?.onError?.(typedError, variables, context); + return options?.onError?.(error, variables, context); }, onSuccess: (updatedAgent, variables, context) => { const listRes = queryClient.getQueryData([ diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 67fa367b15..0cefd5a135 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -554,7 +554,6 @@ "com_ui_agent_var": "{{0}} agent", "com_ui_agent_version": "Version", "com_ui_agent_version_active": "Active Version", - "com_ui_agent_version_duplicate": "Duplicate version detected. This would create a version identical to Version {{versionIndex}}.", "com_ui_agent_version_empty": "No versions available", "com_ui_agent_version_error": "Error fetching versions", "com_ui_agent_version_history": "Version History", diff --git a/packages/data-provider/src/types/mutations.ts b/packages/data-provider/src/types/mutations.ts index 48e34b5d8c..7b1b65ab73 100644 --- a/packages/data-provider/src/types/mutations.ts +++ b/packages/data-provider/src/types/mutations.ts @@ -136,12 +136,7 @@ export type DuplicateVersionError = Error & { }; }; -export type UpdateAgentMutationOptions = MutationOptions< - Agent, - UpdateAgentVariables, - unknown, - DuplicateVersionError ->; +export type UpdateAgentMutationOptions = MutationOptions; export type DuplicateAgentBody = { agent_id: string;