From b5ab32c5ae888aa5a273ba5cfecb54cd078fefc5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 13 Dec 2025 08:29:15 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=AF=20refactor:=20Centralize=20Agent?= =?UTF-8?q?=20Model=20Handling=20Across=20Conversation=20Lifecycle=20(#109?= =?UTF-8?q?56)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: Implement clearModelForNonEphemeralAgent utility for improved agent handling - Introduced clearModelForNonEphemeralAgent function to manage model state for non-ephemeral agents across various components. - Updated ModelSelectorContext to initialize model based on agent type. - Enhanced useNavigateToConvo, useQueryParams, and useSelectMention hooks to clear model for non-ephemeral agents. - Refactored buildDefaultConvo and endpoints utility to ensure proper handling of agent_id and model state. - Improved overall conversation logic and state management for better performance and reliability. * refactor: Enhance useNewConvo hook to improve agent conversation handling - Added logic to skip access checks for existing agent conversations, utilizing localStorage to restore conversations after refresh. - Improved handling of default endpoints for agents based on user access and existing conversation state, ensuring more reliable conversation initialization. * refactor: Update ChatRoute and useAuthRedirect to include user roles - Enhanced ChatRoute to utilize user roles for improved conversation state management. - Modified useAuthRedirect to return user roles alongside authentication status, ensuring roles are available for conditional logic in ChatRoute. - Adjusted conversation initialization logic to depend on the loaded roles, enhancing the reliability of the conversation setup. * refactor: Update BaseClient to handle non-ephemeral agents in conversation logic - Added a check for non-ephemeral agents in BaseClient, modifying the exceptions set to include 'model' when applicable. - Enhanced conversation handling to improve flexibility based on agent type. * test: Add mock for clearModelForNonEphemeralAgent in useQueryParams tests - Introduced a mock for clearModelForNonEphemeralAgent to enhance testing of query parameters related to non-ephemeral agents. - This addition supports improved test coverage and ensures proper handling of model state in relevant scenarios. * refactor: Simplify mocks in useQueryParams tests for improved clarity - Updated the mocking strategy for utilities in useQueryParams tests to use actual implementations where possible, while still suppressing test output for the logger. - Enhanced the mock for tQueryParamsSchema to minimize complexity and avoid unnecessary validation during tests, improving test reliability and maintainability. * refactor: Enhance agent identification logic in BaseClient for improved clarity * chore: Import Constants in families.ts for enhanced functionality --- api/app/clients/BaseClient.js | 7 +++ .../Menus/Endpoints/ModelSelectorContext.tsx | 16 ++++-- .../Conversations/useNavigateToConvo.tsx | 13 ++++- client/src/hooks/Input/useQueryParams.spec.ts | 55 +++++++++---------- client/src/hooks/Input/useQueryParams.ts | 18 +++++- client/src/hooks/Input/useSelectMention.ts | 9 ++- client/src/hooks/useNewConvo.ts | 23 +++++++- client/src/routes/ChatRoute.tsx | 8 ++- client/src/routes/useAuthRedirect.ts | 3 +- client/src/store/families.ts | 14 ++++- client/src/utils/buildDefaultConvo.ts | 11 +++- client/src/utils/endpoints.ts | 33 +++++++++++ 12 files changed, 161 insertions(+), 49 deletions(-) diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index 126efcc385..e85a550e26 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -966,6 +966,13 @@ class BaseClient { const unsetFields = {}; const exceptions = new Set(['spec', 'iconURL']); + const hasNonEphemeralAgent = + isAgentsEndpoint(this.options.endpoint) && + endpointOptions?.agent_id && + endpointOptions.agent_id !== Constants.EPHEMERAL_AGENT_ID; + if (hasNonEphemeralAgent) { + exceptions.add('model'); + } if (existingConvo != null) { this.fetchedConvo = true; for (const key in existingConvo) { diff --git a/client/src/components/Chat/Menus/Endpoints/ModelSelectorContext.tsx b/client/src/components/Chat/Menus/Endpoints/ModelSelectorContext.tsx index e79d9a2d21..26d476e85d 100644 --- a/client/src/components/Chat/Menus/Endpoints/ModelSelectorContext.tsx +++ b/client/src/components/Chat/Menus/Endpoints/ModelSelectorContext.tsx @@ -104,10 +104,18 @@ export function ModelSelectorProvider({ children, startupConfig }: ModelSelector }); // State - const [selectedValues, setSelectedValues] = useState({ - endpoint: endpoint || '', - model: model || '', - modelSpec: spec || '', + const [selectedValues, setSelectedValues] = useState(() => { + let initialModel = model || ''; + if (isAgentsEndpoint(endpoint) && agent_id) { + initialModel = agent_id; + } else if (isAssistantsEndpoint(endpoint) && assistant_id) { + initialModel = assistant_id; + } + return { + endpoint: endpoint || '', + model: initialModel, + modelSpec: spec || '', + }; }); useSelectorEffects({ agentsMap, diff --git a/client/src/hooks/Conversations/useNavigateToConvo.tsx b/client/src/hooks/Conversations/useNavigateToConvo.tsx index bf8321feb1..114b70c6ef 100644 --- a/client/src/hooks/Conversations/useNavigateToConvo.tsx +++ b/client/src/hooks/Conversations/useNavigateToConvo.tsx @@ -9,7 +9,13 @@ import type { TModelsConfig, TConversation, } from 'librechat-data-provider'; -import { getDefaultEndpoint, clearMessagesCache, buildDefaultConvo, logger } from '~/utils'; +import { + clearModelForNonEphemeralAgent, + getDefaultEndpoint, + clearMessagesCache, + buildDefaultConvo, + logger, +} from '~/utils'; import { useApplyModelSpecEffects } from '~/hooks/Agents'; import store from '~/store'; @@ -49,7 +55,10 @@ const useNavigateToConvo = (index = 0) => { dataService.getConversationById(conversationId), ); logger.log('conversation', 'Fetched fresh conversation data', data); - setConversation(data); + + const convoData = { ...data }; + clearModelForNonEphemeralAgent(convoData); + setConversation(convoData); navigate(`/c/${conversationId ?? Constants.NEW_CONVO}`, { state: { focusChat: true } }); } catch (error) { console.error('Error fetching conversation data on navigation', error); diff --git a/client/src/hooks/Input/useQueryParams.spec.ts b/client/src/hooks/Input/useQueryParams.spec.ts index 7b52495cc8..927df94941 100644 --- a/client/src/hooks/Input/useQueryParams.spec.ts +++ b/client/src/hooks/Input/useQueryParams.spec.ts @@ -65,38 +65,33 @@ jest.mock('~/hooks/Agents/useAgentDefaultPermissionLevel', () => ({ default: jest.fn(() => ({})), })); -jest.mock('~/utils', () => ({ - getConvoSwitchLogic: jest.fn(() => ({ - template: {}, - shouldSwitch: false, - isNewModular: false, - newEndpointType: null, - isCurrentModular: false, - isExistingConversation: false, - })), - getModelSpecIconURL: jest.fn(() => 'icon-url'), - removeUnavailableTools: jest.fn((preset) => preset), - logger: { log: jest.fn() }, - getInitialTheme: jest.fn(() => 'light'), - applyFontSize: jest.fn(), -})); +jest.mock('~/utils', () => { + const actualUtils = jest.requireActual('~/utils'); + return { + ...actualUtils, + // Only mock logger to suppress test output + logger: { log: jest.fn(), warn: jest.fn(), error: jest.fn() }, + // Mock theme utilities that interact with DOM + getInitialTheme: jest.fn(() => 'light'), + applyFontSize: jest.fn(), + }; +}); -// Mock the tQueryParamsSchema -jest.mock('librechat-data-provider', () => ({ - ...jest.requireActual('librechat-data-provider'), - tQueryParamsSchema: { - shape: { - model: { parse: jest.fn((value) => value) }, - endpoint: { parse: jest.fn((value) => value) }, - temperature: { parse: jest.fn((value) => value) }, - // Add other schema shapes as needed +// Use actual librechat-data-provider with minimal overrides +jest.mock('librechat-data-provider', () => { + const actual = jest.requireActual('librechat-data-provider'); + return { + ...actual, + // Override schema to avoid complex validation in tests + tQueryParamsSchema: { + shape: { + model: { parse: jest.fn((value) => value) }, + endpoint: { parse: jest.fn((value) => value) }, + temperature: { parse: jest.fn((value) => value) }, + }, }, - }, - isAgentsEndpoint: jest.fn(() => false), - isAssistantsEndpoint: jest.fn(() => false), - QueryKeys: { startupConfig: 'startupConfig', endpoints: 'endpoints' }, - EModelEndpoint: { custom: 'custom', assistants: 'assistants', agents: 'agents' }, -})); + }; +}); // Mock data-provider hooks jest.mock('~/data-provider', () => ({ diff --git a/client/src/hooks/Input/useQueryParams.ts b/client/src/hooks/Input/useQueryParams.ts index d2d1f66a4d..7c9ff58042 100644 --- a/client/src/hooks/Input/useQueryParams.ts +++ b/client/src/hooks/Input/useQueryParams.ts @@ -11,13 +11,19 @@ import { PermissionBits, } from 'librechat-data-provider'; import type { - TPreset, + AgentListResponse, TEndpointsConfig, TStartupConfig, - AgentListResponse, + TPreset, } from 'librechat-data-provider'; import type { ZodAny } from 'zod'; -import { getConvoSwitchLogic, getModelSpecIconURL, removeUnavailableTools, logger } from '~/utils'; +import { + clearModelForNonEphemeralAgent, + removeUnavailableTools, + getModelSpecIconURL, + getConvoSwitchLogic, + logger, +} from '~/utils'; import { useAuthContext, useAgentsMap, useDefaultConvo, useSubmitMessage } from '~/hooks'; import { useChatContext, useChatFormContext } from '~/Providers'; import { useGetAgentByIdQuery } from '~/data-provider'; @@ -194,6 +200,12 @@ export default function useQueryParams({ newPreset = { ...newPreset, ...resetParams }; } + // Sync agent_id from newPreset to template, then clear model if non-ephemeral agent + if (newPreset.agent_id) { + template.agent_id = newPreset.agent_id; + } + clearModelForNonEphemeralAgent(template); + const isModular = isCurrentModular && isNewModular && shouldSwitch; if (isExistingConversation && isModular) { template.endpointType = newEndpointType as EModelEndpoint | undefined; diff --git a/client/src/hooks/Input/useSelectMention.ts b/client/src/hooks/Input/useSelectMention.ts index 51a2f75b11..731302ff0a 100644 --- a/client/src/hooks/Input/useSelectMention.ts +++ b/client/src/hooks/Input/useSelectMention.ts @@ -9,7 +9,13 @@ import type { TEndpointsConfig, } from 'librechat-data-provider'; import type { MentionOption, ConvoGenerator } from '~/common'; -import { getConvoSwitchLogic, getModelSpecIconURL, removeUnavailableTools, logger } from '~/utils'; +import { + clearModelForNonEphemeralAgent, + removeUnavailableTools, + getModelSpecIconURL, + getConvoSwitchLogic, + logger, +} from '~/utils'; import { useDefaultConvo } from '~/hooks'; import store from '~/store'; @@ -154,6 +160,7 @@ export default function useSelectMention({ if (agent_id) { template.agent_id = agent_id; } + clearModelForNonEphemeralAgent(template); template.spec = null; template.iconURL = null; diff --git a/client/src/hooks/useNewConvo.ts b/client/src/hooks/useNewConvo.ts index e6945fe6bc..f48f172072 100644 --- a/client/src/hooks/useNewConvo.ts +++ b/client/src/hooks/useNewConvo.ts @@ -24,6 +24,7 @@ import type { import type { AssistantListItem } from '~/common'; import { updateLastSelectedModel, + getLocalStorageItems, getDefaultModelSpec, getDefaultEndpoint, getModelSpecPreset, @@ -112,7 +113,21 @@ const useNewConvo = (index = 0) => { }); // If the selected endpoint is agents but user doesn't have access, find an alternative - if (defaultEndpoint && isAgentsEndpoint(defaultEndpoint) && !hasAgentAccess) { + // Skip this check for existing agent conversations (they have agent_id set) + // Also check localStorage for new conversations restored after refresh + const { lastConversationSetup } = getLocalStorageItems(); + const storedAgentId = + isAgentsEndpoint(lastConversationSetup?.endpoint) && lastConversationSetup?.agent_id; + const isExistingAgentConvo = + isAgentsEndpoint(defaultEndpoint) && + ((conversation.agent_id && conversation.agent_id !== Constants.EPHEMERAL_AGENT_ID) || + (storedAgentId && storedAgentId !== Constants.EPHEMERAL_AGENT_ID)); + if ( + defaultEndpoint && + isAgentsEndpoint(defaultEndpoint) && + !hasAgentAccess && + !isExistingAgentConvo + ) { defaultEndpoint = Object.keys(endpointsConfig ?? {}).find( (ep) => !isAgentsEndpoint(ep as EModelEndpoint) && endpointsConfig?.[ep], ) as EModelEndpoint | undefined; @@ -121,7 +136,11 @@ const useNewConvo = (index = 0) => { if (!defaultEndpoint) { // Find first available endpoint that's not agents (if no access) or any endpoint defaultEndpoint = Object.keys(endpointsConfig ?? {}).find((ep) => { - if (isAgentsEndpoint(ep as EModelEndpoint) && !hasAgentAccess) { + if ( + isAgentsEndpoint(ep as EModelEndpoint) && + !hasAgentAccess && + !isExistingAgentConvo + ) { return false; } return !!endpointsConfig?.[ep]; diff --git a/client/src/routes/ChatRoute.tsx b/client/src/routes/ChatRoute.tsx index 72433bca46..0670ee1e85 100644 --- a/client/src/routes/ChatRoute.tsx +++ b/client/src/routes/ChatRoute.tsx @@ -16,7 +16,7 @@ import store from '~/store'; export default function ChatRoute() { const { data: startupConfig } = useGetStartupConfig(); - const { isAuthenticated, user } = useAuthRedirect(); + const { isAuthenticated, user, roles } = useAuthRedirect(); const defaultTemporaryChat = useRecoilValue(temporaryStore.defaultTemporaryChat); const setIsTemporary = useRecoilCallback( @@ -61,8 +61,11 @@ export default function ChatRoute() { * Adjusting this may have unintended consequences on the conversation state. */ useEffect(() => { + // Wait for roles to load so hasAgentAccess has a definitive value in useNewConvo + const rolesLoaded = roles?.USER != null; const shouldSetConvo = - (startupConfig && !hasSetConversation.current && !modelsQuery.data?.initial) ?? false; + (startupConfig && rolesLoaded && !hasSetConversation.current && !modelsQuery.data?.initial) ?? + false; /* Early exit if startupConfig is not loaded and conversation is already set and only initial models have loaded */ if (!shouldSetConvo) { return; @@ -119,6 +122,7 @@ export default function ChatRoute() { /* Creates infinite render if all dependencies included due to newConversation invocations exceeding call stack before hasSetConversation.current becomes truthy */ // eslint-disable-next-line react-hooks/exhaustive-deps }, [ + roles, startupConfig, initialConvoQuery.data, endpointsQuery.data, diff --git a/client/src/routes/useAuthRedirect.ts b/client/src/routes/useAuthRedirect.ts index f4d2d8588d..86d8103384 100644 --- a/client/src/routes/useAuthRedirect.ts +++ b/client/src/routes/useAuthRedirect.ts @@ -3,7 +3,7 @@ import { useNavigate } from 'react-router-dom'; import { useAuthContext } from '~/hooks'; export default function useAuthRedirect() { - const { user, isAuthenticated } = useAuthContext(); + const { user, roles, isAuthenticated } = useAuthContext(); const navigate = useNavigate(); useEffect(() => { @@ -20,6 +20,7 @@ export default function useAuthRedirect() { return { user, + roles, isAuthenticated, }; } diff --git a/client/src/store/families.ts b/client/src/store/families.ts index 0d630296db..42a4a8b155 100644 --- a/client/src/store/families.ts +++ b/client/src/store/families.ts @@ -1,4 +1,5 @@ import { useEffect } from 'react'; +import { createSearchParams } from 'react-router-dom'; import { atom, selector, @@ -13,9 +14,13 @@ import { import { LocalStorageKeys, Constants } from 'librechat-data-provider'; import type { TMessage, TPreset, TConversation, TSubmission } from 'librechat-data-provider'; import type { TOptionSettings, ExtendedFile } from '~/common'; +import { + clearModelForNonEphemeralAgent, + createChatSearchParams, + storeEndpointSettings, + logger, +} from '~/utils'; import { useSetConvoContext } from '~/Providers/SetConvoContext'; -import { storeEndpointSettings, logger, createChatSearchParams } from '~/utils'; -import { createSearchParams } from 'react-router-dom'; const latestMessageKeysAtom = atom<(string | number)[]>({ key: 'latestMessageKeys', @@ -101,9 +106,12 @@ const conversationByIndex = atomFamily({ } storeEndpointSettings(newValue); + + const convoToStore = { ...newValue }; + clearModelForNonEphemeralAgent(convoToStore); localStorage.setItem( `${LocalStorageKeys.LAST_CONVO_SETUP}_${index}`, - JSON.stringify(newValue), + JSON.stringify(convoToStore), ); const disableParams = newValue.disableParams === true; diff --git a/client/src/utils/buildDefaultConvo.ts b/client/src/utils/buildDefaultConvo.ts index 497013e763..acfb0873b9 100644 --- a/client/src/utils/buildDefaultConvo.ts +++ b/client/src/utils/buildDefaultConvo.ts @@ -1,10 +1,12 @@ import { + Constants, parseConvo, EModelEndpoint, isAssistantsEndpoint, isAgentsEndpoint, } from 'librechat-data-provider'; import type { TConversation, EndpointSchemaKey } from 'librechat-data-provider'; +import { clearModelForNonEphemeralAgent } from './endpoints'; import { getLocalStorageItems } from './localStorage'; const buildDefaultConvo = ({ @@ -66,10 +68,17 @@ const buildDefaultConvo = ({ // Ensures agent_id is always defined const agentId = convo?.agent_id ?? ''; const defaultAgentId = lastConversationSetup?.agent_id ?? ''; - if (isAgentsEndpoint(endpoint) && !defaultAgentId && agentId) { + if ( + isAgentsEndpoint(endpoint) && + agentId && + (!defaultAgentId || defaultAgentId === Constants.EPHEMERAL_AGENT_ID) + ) { defaultConvo.agent_id = agentId; } + // Clear model for non-ephemeral agents - agents use their configured model internally + clearModelForNonEphemeralAgent(defaultConvo); + defaultConvo.tools = lastConversationSetup?.tools ?? lastSelectedTools ?? defaultConvo.tools; return defaultConvo; diff --git a/client/src/utils/endpoints.ts b/client/src/utils/endpoints.ts index 3702fb00ca..ffe4b1b608 100644 --- a/client/src/utils/endpoints.ts +++ b/client/src/utils/endpoints.ts @@ -11,6 +11,27 @@ import { import type * as t from 'librechat-data-provider'; import type { LocalizeFunction, IconsRecord } from '~/common'; +/** + * Clears model for non-ephemeral agent conversations. + * Agents use their configured model internally, so the conversation model should be undefined. + * Mutates the template in place. + */ +export function clearModelForNonEphemeralAgent< + T extends { + endpoint?: EModelEndpoint | string | null; + agent_id?: string | null; + model?: string | null; + }, +>(template: T): void { + if ( + isAgentsEndpoint(template.endpoint) && + template.agent_id && + template.agent_id !== Constants.EPHEMERAL_AGENT_ID + ) { + template.model = undefined as T['model']; + } +} + export const getEntityName = ({ name = '', localize, @@ -125,6 +146,18 @@ export function getConvoSwitchLogic(params: ConversationInitParams): InitiatedTe conversationId: 'new', }; + // Reset agent_id if switching to a non-agents endpoint but template has a non-ephemeral agent_id + if ( + !isAgentsEndpoint(newEndpoint) && + template.agent_id && + template.agent_id !== Constants.EPHEMERAL_AGENT_ID + ) { + template.agent_id = Constants.EPHEMERAL_AGENT_ID; + } + + // Clear model for non-ephemeral agents - agents use their configured model internally + clearModelForNonEphemeralAgent(template); + const isAssistantSwitch = isAssistantsEndpoint(newEndpoint) && isAssistantsEndpoint(currentEndpoint) &&