Skip to content

Conversation

@changkun
Copy link

Summary

In #10634, users can pin their favorite models on the sidebar. However, when users start a new conversation, the interface jumps back to a default configured model/agent (if any). For example,

modelSpecs:
  enforce: false
  prioritize: false
  list:
    - name: "my-agent"
      label: "My Agent"
      description: "Default Assistant."
      showIconInMenu: true
      showIconInHeader: true
      default: true
      preset:
        endpoint: "agents"
        agent_id: "agent_abcd"
        modelLabel: "My Agent"

This setting gives new users a default setup for all kinds of activities, but as soon as they start exploring other models and building their own customized agents, we observe that users prefer to stick to their last choice, and the pinned model does not fully unlock this capability.

In this PR, we enhanced the model choice behavior when starting a new conversation, allowing new conversations to use the last chosen model/agent (persisted in storage).

Change Type

Please delete any irrelevant options.

  • New feature (non-breaking change which adds functionality)

Testing

Manually tested. Procedure:

  • Select a model/agent
  • Start a new conversation

The new conversation starts with the previously selected model/agent.

Checklist

Please delete any irrelevant options.

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • Local unit tests pass with my changes
  • Any changes dependent on mine have been merged and published in downstream modules.

@changkun changkun changed the base branch from main to dev December 27, 2025 09:44
@changkun changkun marked this pull request as ready for review December 27, 2025 09:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the model/agent selection behavior when starting new conversations by remembering and reusing the user's last selected model/agent instead of always defaulting to an admin-configured default. This improves user experience for those who have explored various models and built customized agents by maintaining their preferences across conversation sessions.

Key Changes

  • Modified the priority logic to prefer last selected model/agent over admin-defined defaults
  • Enhanced getDefaultModelSpec to return both admin default and last selected specs, allowing consumers to choose priority
  • Updated useNewConvo to check for last used agent_id (including those outside modelSpecs) and apply appropriate preset selection logic

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File Description
client/src/utils/endpoints.ts Refactored getDefaultModelSpec to return both default and last selected specs with a fallback to reading from LAST_CONVO_SETUP, and updated priority logic to prefer last selected over default
client/src/routes/ChatRoute.tsx Removed modelSpec preset logic and delegated full preset determination to useNewConvo hook
client/src/hooks/useNewConvo.ts Added complex logic to prefer last used specs/agents over defaults, including handling for agents outside modelSpecs, and added index to dependency array

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 298 to +335
let preset = _preset;
const result = getDefaultModelSpec(startupConfig);
const defaultModelSpec = result?.default ?? result?.last;
if (

// Prefer last used spec over default spec for better user experience
const defaultModelSpec = result?.last ?? result?.default;

// Check for last used agent_id that might be outside of modelSpecs
const lastAgentId = localStorage.getItem(`${LocalStorageKeys.AGENT_ID_PREFIX}${index}`);

// Get the last conversation setup to determine what was most recently selected
// This is the most reliable way to know if a custom agent was selected after a modelSpec
const lastConvoSetup = JSON.parse(
localStorage.getItem(`${LocalStorageKeys.LAST_CONVO_SETUP}_${index}`) ?? '{}',
);
const lastConvoAgentId = lastConvoSetup?.agent_id;
const lastConvoSpec = lastConvoSetup?.spec;

// Use lastAgentId if:
// 1. No preset passed AND no template AND lastAgentId exists
// 2. AND the last conversation had this agent_id without a spec (custom agent selection)
// OR there's no defaultModelSpec at all
// This ensures:
// - Custom agent selection (no spec) is remembered
// - ModelSpec selections (has spec) take priority when they were the last action
const shouldUseLastAgent =
!_preset &&
lastAgentId &&
Object.keys(_template).length === 0 &&
((!lastConvoSpec && lastConvoAgentId === lastAgentId) || !defaultModelSpec);

if (shouldUseLastAgent) {
// Create a preset for the last used agent outside of modelSpecs
preset = {
endpoint: EModelEndpoint.agents,
agent_id: lastAgentId,
};
logger.log('conversation', 'Using last used agent_id outside modelSpecs', lastAgentId);
} else if (
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new logic for determining preset priority is complex and lacks test coverage. Given that this is a critical user-facing feature (remembering last selected model/agent), consider adding unit tests to verify the behavior in various scenarios: when lastAgentId exists but no lastConvoSpec, when both exist, when neither exist, when modelSpecs prioritize is true/false, etc.

Copilot uses AI. Check for mistakes.
Comment on lines 246 to +270
const defaultSpec = list?.find((spec) => spec.default);
if (prioritize === true || !interfaceConfig?.modelSelect) {
const lastSelectedSpecName = localStorage.getItem(LocalStorageKeys.LAST_SPEC);
const lastSelectedSpec = list?.find((spec) => spec.name === lastSelectedSpecName);
return { default: defaultSpec || lastSelectedSpec || list?.[0] };
} else if (defaultSpec) {
return { default: defaultSpec };

// Try to get last selected spec from localStorage
const lastSelectedSpecName = localStorage.getItem(LocalStorageKeys.LAST_SPEC);
let lastSelectedSpec = lastSelectedSpecName
? list?.find((spec) => spec.name === lastSelectedSpecName)
: undefined;

// Fallback: try to get spec from last conversation setup
if (!lastSelectedSpec) {
const lastConversationSetup = JSON.parse(
localStorage.getItem(LocalStorageKeys.LAST_CONVO_SETUP + '_0') ?? '{}',
);
if (lastConversationSetup.spec) {
lastSelectedSpec = list?.find((spec) => spec.name === lastConversationSetup.spec);
}
}
const lastConversationSetup = JSON.parse(
localStorage.getItem(LocalStorageKeys.LAST_CONVO_SETUP + '_0') ?? '{}',
);
if (!lastConversationSetup.spec) {
return;

if (prioritize === true || !interfaceConfig?.modelSelect) {
// When prioritize is true or modelSelect is disabled, return the effective spec as default
return { default: lastSelectedSpec || defaultSpec || list?.[0], last: lastSelectedSpec };
}
return { last: list?.find((spec) => spec.name === lastConversationSetup.spec) };

// Return both default and last, allowing consumer to choose priority
return { default: defaultSpec, last: lastSelectedSpec };
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modified getDefaultModelSpec function now has more complex fallback logic (checking both LAST_SPEC and LAST_CONVO_SETUP), but lacks test coverage. Consider adding tests to verify the new behavior, especially the fallback mechanism and priority logic when prioritize is enabled vs disabled.

Copilot uses AI. Check for mistakes.
Comment on lines +256 to +258
const lastConversationSetup = JSON.parse(
localStorage.getItem(LocalStorageKeys.LAST_CONVO_SETUP + '_0') ?? '{}',
);
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for JSON.parse. If localStorage contains corrupted data, this will throw an exception and break the function. Consider wrapping in a try-catch block or using a safe parsing utility, similar to how other parts of the codebase handle this (e.g., line 107-108 where there's a double fallback with ?? '{}' || '{}').

Suggested change
const lastConversationSetup = JSON.parse(
localStorage.getItem(LocalStorageKeys.LAST_CONVO_SETUP + '_0') ?? '{}',
);
let lastConversationSetup: { spec?: string } = {};
try {
const storedSetup = localStorage.getItem(LocalStorageKeys.LAST_CONVO_SETUP + '_0');
lastConversationSetup = storedSetup ? JSON.parse(storedSetup) : {};
} catch {
lastConversationSetup = {};
}

Copilot uses AI. Check for mistakes.
Comment on lines +309 to +311
const lastConvoSetup = JSON.parse(
localStorage.getItem(`${LocalStorageKeys.LAST_CONVO_SETUP}_${index}`) ?? '{}',
);
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for JSON.parse. If localStorage contains corrupted data for LAST_CONVO_SETUP, this will throw an exception and break the conversation creation flow. Consider wrapping in a try-catch block to handle corrupted data gracefully, defaulting to an empty object on parse errors.

Suggested change
const lastConvoSetup = JSON.parse(
localStorage.getItem(`${LocalStorageKeys.LAST_CONVO_SETUP}_${index}`) ?? '{}',
);
const lastConvoSetupRaw =
localStorage.getItem(`${LocalStorageKeys.LAST_CONVO_SETUP}_${index}`) ?? '{}';
let lastConvoSetup: any = {};
try {
lastConvoSetup = JSON.parse(lastConvoSetupRaw);
} catch (error) {
logger?.warn?.('Failed to parse LAST_CONVO_SETUP from localStorage', error);
lastConvoSetup = {};
}

Copilot uses AI. Check for mistakes.
Comment on lines +322 to +326
const shouldUseLastAgent =
!_preset &&
lastAgentId &&
Object.keys(_template).length === 0 &&
((!lastConvoSpec && lastConvoAgentId === lastAgentId) || !defaultModelSpec);
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complex conditional logic in shouldUseLastAgent is difficult to understand and maintain. The condition checks for multiple scenarios but the logic flow is not immediately clear. Consider breaking this down into smaller, well-named boolean variables that explain each condition (e.g., isCustomAgentSelection, wasLastActionModelSpec, hasNoDefaultSpec) to improve readability and maintainability.

Suggested change
const shouldUseLastAgent =
!_preset &&
lastAgentId &&
Object.keys(_template).length === 0 &&
((!lastConvoSpec && lastConvoAgentId === lastAgentId) || !defaultModelSpec);
const noPresetProvided = !_preset;
const hasLastAgentId = Boolean(lastAgentId);
const hasNoTemplate = Object.keys(_template).length === 0;
const wasCustomAgentSelection = !lastConvoSpec && lastConvoAgentId === lastAgentId;
const hasNoDefaultSpec = !defaultModelSpec;
const shouldUseLastAgent =
noPresetProvided &&
hasLastAgentId &&
hasNoTemplate &&
(wasCustomAgentSelection || hasNoDefaultSpec);

Copilot uses AI. Check for mistakes.
const shouldUseLastAgent =
!_preset &&
lastAgentId &&
Object.keys(_template).length === 0 &&
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition Object.keys(_template).length === 0 is used to determine if a template is empty, but this doesn't account for templates that have properties with undefined or null values. Consider using a more robust check or documenting why this specific check is sufficient for this use case.

Copilot uses AI. Check for mistakes.
Comment on lines +232 to +233
* Returns both admin default and last selected spec when available.
* Priority for usage: last selected → admin default → first spec (when prioritize=true or modelSelect disabled).
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Returns both admin default and last selected spec when available" but the function can return undefined, only a default, only a last, or both. The documentation should clarify all possible return states and when each occurs to help consumers understand the function's behavior.

Suggested change
* Returns both admin default and last selected spec when available.
* Priority for usage: last selected admin default first spec (when prioritize=true or modelSelect disabled).
*
* Return shape:
* - `undefined` when there is no `modelSpecs.list` in the startup config.
* - An object `{ default?: TModelSpec; last?: TModelSpec }` where:
* - `last` is the most recently selected spec (from localStorage), if found.
* - `default` is:
* - When `prioritize === true` or `interface.modelSelect === false`:
* the effective spec following the priority
* `last selected → admin default → first spec in list`.
* - Otherwise: the admin-default spec (the one marked `default`), if any.
*
* Note: In all cases, `default` and `last` may be `undefined` individually if
* no matching specs are found for those roles.

Copilot uses AI. Check for mistakes.
Comment on lines +301 to +302
// Prefer last used spec over default spec for better user experience
const defaultModelSpec = result?.last ?? result?.default;
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The priority logic has changed from "default → last" to "last → default" (line 302), but when prioritize is true or modelSelect is disabled, the function returns the effective spec in the default field (line 266). This could be confusing for consumers who expect the default field to always contain the admin-defined default. Consider either using a different field name like effective or ensuring consistent semantics across return values.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant