mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-06-15 23:43:06 +03:00
🔌 fix: Preserve Ephemeral MCP Selections Across Model Switches (#13697)
The no-spec branch of `useApplyModelSpecEffects` (added in #11796) reset `ephemeralAgentByConvoId` to null on every `newConversation` call when model specs are configured. On in-place model/endpoint switches (modular chat, same conversation or new-chat draft), BadgeRowContext never refills from localStorage — its init effect only re-runs when the storage suffix or spec changes — so the MCP selection (and tool toggles) were silently dropped from subsequent request payloads while the MCP badge kept displaying them. Reset now only happens on context transitions (leaving a spec, or moving to a different conversation key), where a BadgeRowContext refill is guaranteed; in-place non-spec switches preserve the ephemeral agent. - Gate the no-spec reset on `prevSpecName` / `prevConvoId`, passed from `newConversation` via a snapshot read of the pre-switch conversation - Add jest coverage for all five branches of the no-spec path - Add e2e spec asserting `ephemeralAgent.mcp` stays in the chat payload after a new-chat model switch and after regenerate on a switched conversation (verified failing before the fix, passing after) - Add non-spec "Mock Provider D" endpoint to the e2e config so tests can switch between two real ephemeral endpoints; widen `MockEndpoint` type
This commit is contained in:
@@ -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 }) => (
|
||||
<RecoilRoot>{children}</RecoilRoot>
|
||||
);
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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'
|
||||
|
||||
@@ -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';
|
||||
|
||||
|
||||
84
e2e/specs/mock/mcp-ephemeral.spec.ts
Normal file
84
e2e/specs/mock/mcp-ephemeral.spec.ts
Normal file
@@ -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 });
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user