diff --git a/client/src/hooks/Agents/__tests__/useApplyModelSpecAgents.spec.tsx b/client/src/hooks/Agents/__tests__/useApplyModelSpecAgents.spec.tsx new file mode 100644 index 0000000000..ce68d560b3 --- /dev/null +++ b/client/src/hooks/Agents/__tests__/useApplyModelSpecAgents.spec.tsx @@ -0,0 +1,147 @@ +import React from 'react'; +import { RecoilRoot, useRecoilValue } from 'recoil'; +import { renderHook, act } from '@testing-library/react'; +import { Constants, EModelEndpoint } from 'librechat-data-provider'; +import type { TEphemeralAgent, TStartupConfig, TModelSpec } from 'librechat-data-provider'; +import { ephemeralAgentByConvoId, useUpdateEphemeralAgent } from '~/store/agents'; +import { useApplyModelSpecEffects } from '../useApplyModelSpecAgents'; + +const NEW_CONVO = Constants.NEW_CONVO as string; + +const Wrapper = ({ children }: { children: React.ReactNode }) => ( + {children} +); + +const createModelSpec = (name: string): TModelSpec => + ({ + name, + label: name, + preset: { + endpoint: EModelEndpoint.openAI, + model: name, + }, + }) as TModelSpec; + +const createStartupConfig = (list: TModelSpec[]): TStartupConfig => + ({ + modelSpecs: { + list, + prioritize: false, + }, + }) as TStartupConfig; + +const specsConfig = () => createStartupConfig([createModelSpec('test-spec')]); + +const useHarness = (conversationId: string) => { + const applyModelSpecEffects = useApplyModelSpecEffects(); + const updateEphemeralAgent = useUpdateEphemeralAgent(); + const ephemeralAgent = useRecoilValue(ephemeralAgentByConvoId(conversationId)); + return { applyModelSpecEffects, updateEphemeralAgent, ephemeralAgent }; +}; + +describe('useApplyModelSpecEffects', () => { + it('preserves an existing conversation ephemeral agent on an in-place model switch', () => { + const conversationId = 'convo-123'; + const agent: TEphemeralAgent = { mcp: ['clickhouse'] }; + const { result } = renderHook(() => useHarness(conversationId), { wrapper: Wrapper }); + + act(() => { + result.current.updateEphemeralAgent(conversationId, agent); + }); + expect(result.current.ephemeralAgent).toEqual(agent); + + act(() => { + result.current.applyModelSpecEffects({ + convoId: conversationId, + specName: null, + prevConvoId: conversationId, + prevSpecName: null, + startupConfig: specsConfig(), + }); + }); + + expect(result.current.ephemeralAgent).toEqual(agent); + }); + + it('preserves a new conversation ephemeral agent on an in-place model switch', () => { + const agent: TEphemeralAgent = { mcp: ['clickhouse'] }; + const { result } = renderHook(() => useHarness(NEW_CONVO), { wrapper: Wrapper }); + + act(() => { + result.current.updateEphemeralAgent(NEW_CONVO, agent); + }); + + act(() => { + result.current.applyModelSpecEffects({ + convoId: NEW_CONVO, + specName: null, + prevConvoId: NEW_CONVO, + prevSpecName: null, + startupConfig: specsConfig(), + }); + }); + + expect(result.current.ephemeralAgent).toEqual(agent); + }); + + it('resets the ephemeral agent when switching away from a spec', () => { + const { result } = renderHook(() => useHarness(NEW_CONVO), { wrapper: Wrapper }); + + act(() => { + result.current.updateEphemeralAgent(NEW_CONVO, { mcp: ['clickhouse'] }); + }); + + act(() => { + result.current.applyModelSpecEffects({ + convoId: NEW_CONVO, + specName: null, + prevConvoId: NEW_CONVO, + prevSpecName: 'test-spec', + startupConfig: specsConfig(), + }); + }); + + expect(result.current.ephemeralAgent).toBeNull(); + }); + + it('resets the new conversation ephemeral agent when leaving an existing conversation', () => { + const { result } = renderHook(() => useHarness(NEW_CONVO), { wrapper: Wrapper }); + + act(() => { + result.current.updateEphemeralAgent(NEW_CONVO, { mcp: ['clickhouse'] }); + }); + + act(() => { + result.current.applyModelSpecEffects({ + convoId: NEW_CONVO, + specName: null, + prevConvoId: 'convo-123', + prevSpecName: null, + startupConfig: specsConfig(), + }); + }); + + expect(result.current.ephemeralAgent).toBeNull(); + }); + + it('leaves the ephemeral agent untouched when no specs are configured', () => { + const agent: TEphemeralAgent = { mcp: ['clickhouse'] }; + const { result } = renderHook(() => useHarness(NEW_CONVO), { wrapper: Wrapper }); + + act(() => { + result.current.updateEphemeralAgent(NEW_CONVO, agent); + }); + + act(() => { + result.current.applyModelSpecEffects({ + convoId: NEW_CONVO, + specName: null, + prevConvoId: 'convo-123', + prevSpecName: 'test-spec', + startupConfig: {} as TStartupConfig, + }); + }); + + expect(result.current.ephemeralAgent).toEqual(agent); + }); +}); diff --git a/client/src/hooks/Agents/useApplyModelSpecAgents.ts b/client/src/hooks/Agents/useApplyModelSpecAgents.ts index 2c677f85ca..3b33968e86 100644 --- a/client/src/hooks/Agents/useApplyModelSpecAgents.ts +++ b/client/src/hooks/Agents/useApplyModelSpecAgents.ts @@ -10,7 +10,11 @@ import { getModelSpec, applyModelSpecEphemeralAgent } from '~/utils'; * * When a spec is provided, its tool settings are applied to the ephemeral agent. * When no spec is provided but specs are configured, the ephemeral agent is reset - * to null so BadgeRowContext can apply localStorage defaults (non-spec experience). + * to null on context transitions (leaving a spec, or moving to a different + * conversation key) so BadgeRowContext refills values from localStorage — both + * transitions re-trigger its init effect. In-place switches (same conversation, + * non-spec → non-spec) keep the ephemeral agent state (e.g. MCP selections), + * since no refill would follow the reset. */ export function useApplyModelSpecEffects() { const updateEphemeralAgent = useUpdateEphemeralAgent(); @@ -18,17 +22,25 @@ export function useApplyModelSpecEffects() { ({ convoId, specName, + prevConvoId, + prevSpecName, startupConfig, }: { convoId: string | null; specName?: string | null; + prevConvoId?: string | null; + prevSpecName?: string | null; startupConfig?: TStartupConfig; }) => { if (specName == null || !specName) { - if (startupConfig?.modelSpecs?.list?.length) { - /** Specs are configured but none selected — reset ephemeral agent to null - * so BadgeRowContext fills all values (tool toggles + MCP) from localStorage. */ - updateEphemeralAgent((convoId ?? Constants.NEW_CONVO) || Constants.NEW_CONVO, null); + if (!startupConfig?.modelSpecs?.list?.length) { + return; + } + const targetId = (convoId ?? Constants.NEW_CONVO) || Constants.NEW_CONVO; + const sourceId = (prevConvoId ?? Constants.NEW_CONVO) || Constants.NEW_CONVO; + const isContextSwitch = Boolean(prevSpecName) || targetId !== sourceId; + if (isContextSwitch) { + updateEphemeralAgent(targetId, null); } return; } diff --git a/client/src/hooks/useNewConvo.ts b/client/src/hooks/useNewConvo.ts index de58d54165..fb34897a22 100644 --- a/client/src/hooks/useNewConvo.ts +++ b/client/src/hooks/useNewConvo.ts @@ -35,6 +35,7 @@ import { logger, } from '~/utils'; import { useDeleteFilesMutation, useGetEndpointsQuery, useGetStartupConfig } from '~/data-provider'; +import useGetConversation from './Conversations/useGetConversation'; import useAssistantListMap from './Assistants/useAssistantListMap'; import { useResetChatBadges } from './useChatBadges'; import { useApplyModelSpecEffects } from './Agents'; @@ -46,6 +47,7 @@ const useNewConvo = (index = 0) => { const navigate = useNavigate(); const [searchParams] = useSearchParams(); const { data: startupConfig } = useGetStartupConfig(); + const getConversation = useGetConversation(index); const applyModelSpecEffects = useApplyModelSpecEffects(); const clearAllConversations = store.useClearConvoState(); const defaultPreset = useRecoilValue(store.defaultPreset); @@ -334,10 +336,13 @@ const useNewConvo = (index = 0) => { preset = getModelSpecPreset(defaultModelSpec); } + const prevConversation = getConversation(); applyModelSpecEffects({ startupConfig, specName: preset?.spec, convoId: conversation.conversationId, + prevConvoId: prevConversation?.conversationId, + prevSpecName: prevConversation?.spec, }); if (conversation.conversationId === Constants.NEW_CONVO && !modelsData) { @@ -384,6 +389,7 @@ const useNewConvo = (index = 0) => { startupConfig, saveBadgesState, endpointsConfig, + getConversation, pauseGlobalAudio, switchToConversation, applyModelSpecEffects, diff --git a/e2e/config/librechat.e2e.yaml b/e2e/config/librechat.e2e.yaml index 34eeb8b074..89770dcb13 100644 --- a/e2e/config/librechat.e2e.yaml +++ b/e2e/config/librechat.e2e.yaml @@ -50,6 +50,18 @@ endpoints: titleConvo: false modelDisplayLabel: 'Mock Provider C' + # Second non-spec endpoint so e2e tests can switch between two real + # ephemeral endpoints (e.g. MCP selection persistence across switches). + - name: 'Mock Provider D' + apiKey: 'e2e-mock-key-d' + baseURL: 'http://127.0.0.1:8889/v1' + models: + default: + - 'mock-model-d' + fetch: false + titleConvo: false + modelDisplayLabel: 'Mock Provider D' + modelSpecs: prioritize: true # Enforcement would reject sends from the non-spec paths addedEndpoints @@ -59,6 +71,7 @@ modelSpecs: # set) limited to entries that don't collide with the spec labels above. addedEndpoints: - 'Mock Provider C' + - 'Mock Provider D' - 'agents' list: - name: 'e2e-mock-provider-a' diff --git a/e2e/specs/mock/helpers.ts b/e2e/specs/mock/helpers.ts index 42d98eaa82..d24a021c0a 100644 --- a/e2e/specs/mock/helpers.ts +++ b/e2e/specs/mock/helpers.ts @@ -10,7 +10,7 @@ export const MOCK_ENDPOINTS = [ { label: 'Mock Provider B', model: 'mock-model-b' }, ] as const; -export type MockEndpoint = (typeof MOCK_ENDPOINTS)[number]; +export type MockEndpoint = { label: string; model: string }; export const NEW_CHAT_PATH = '/c/new'; diff --git a/e2e/specs/mock/mcp-ephemeral.spec.ts b/e2e/specs/mock/mcp-ephemeral.spec.ts new file mode 100644 index 0000000000..93104f4a43 --- /dev/null +++ b/e2e/specs/mock/mcp-ephemeral.spec.ts @@ -0,0 +1,84 @@ +import { expect, test } from '@playwright/test'; +import type { Page, Response } from '@playwright/test'; +import { + NEW_CHAT_PATH, + isAgentsStream, + mockReply, + selectMockEndpoint, + sendMessage, +} from './helpers'; + +/** Non-spec endpoints from e2e/config/librechat.e2e.yaml — switching between + * them exercises the no-spec `applyModelSpecEffects` path that previously + * wiped the ephemeral agent (MCP selection) for the active conversation. */ +const PROVIDER_C = { label: 'Mock Provider C', model: 'mock-model-c' }; +const PROVIDER_D = { label: 'Mock Provider D', model: 'mock-model-d' }; + +const MCP_SERVER_NAME = 'e2e-memory'; +const MCP_SERVER_TITLE = 'E2E Memory'; + +const uniqueText = (prefix: string) => `${prefix} ${Date.now()}-${Math.floor(Math.random() * 1e4)}`; + +const mcpBadge = (page: Page) => page.getByRole('button', { name: new RegExp(MCP_SERVER_TITLE) }); + +/** Select the MCP server from the composer's ephemeral MCP dropdown. */ +async function selectEphemeralMCP(page: Page) { + await page.getByRole('button', { name: 'MCP Servers', exact: true }).click(); + const serverItem = page.getByRole('menuitemcheckbox', { name: new RegExp(MCP_SERVER_TITLE) }); + await expect(serverItem).toBeVisible(); + await serverItem.click(); + await expect(serverItem).toHaveAttribute('aria-checked', 'true'); + await page.keyboard.press('Escape'); + await expect(mcpBadge(page)).toBeVisible(); +} + +/** The `ephemeralAgent.mcp` array sent with a chat request. */ +function requestMCP(response: Response): string[] | undefined { + const body = response.request().postDataJSON() as { ephemeralAgent?: { mcp?: string[] } }; + return body.ephemeralAgent?.mcp; +} + +test.describe('ephemeral MCP selection persistence', () => { + test('keeps the MCP selection when switching models on a new chat', async ({ page }) => { + test.setTimeout(120000); + await page.goto(NEW_CHAT_PATH, { timeout: 10000 }); + + await selectMockEndpoint(page, PROVIDER_C); + await selectEphemeralMCP(page); + + await selectMockEndpoint(page, PROVIDER_D); + await expect(mcpBadge(page)).toBeVisible(); + + const response = await sendMessage(page, uniqueText('mcp new chat switch')); + expect(response.ok()).toBeTruthy(); + expect(requestMCP(response)).toContain(MCP_SERVER_NAME); + + await expect(mockReply(page)).toBeVisible({ timeout: 20000 }); + }); + + test('keeps the MCP selection when regenerating after a model switch', async ({ page }) => { + test.setTimeout(120000); + await page.goto(NEW_CHAT_PATH, { timeout: 10000 }); + + await selectMockEndpoint(page, PROVIDER_C); + await selectEphemeralMCP(page); + + const first = await sendMessage(page, uniqueText('mcp regenerate switch')); + expect(first.ok()).toBeTruthy(); + expect(requestMCP(first)).toContain(MCP_SERVER_NAME); + await expect(mockReply(page)).toBeVisible({ timeout: 20000 }); + await expect(page).toHaveURL(/\/c\/(?!new)/, { timeout: 15000 }); + + await selectMockEndpoint(page, PROVIDER_D); + await expect(mcpBadge(page)).toBeVisible(); + + const [regenerated] = await Promise.all([ + page.waitForResponse(isAgentsStream, { timeout: 30000 }), + page.getByRole('button', { name: 'Regenerate', exact: true }).click(), + ]); + expect(regenerated.ok()).toBeTruthy(); + expect(requestMCP(regenerated)).toContain(MCP_SERVER_NAME); + + await expect(mockReply(page).first()).toBeVisible({ timeout: 20000 }); + }); +});