Conversation
📝 WalkthroughWalkthroughAdds global defaultModel and defaultVisionModel settings, presenter APIs to get/set them, UI to manage defaults, renderer/main flows to apply defaults for new conversations (excluding ACP), and refactors ImageServer to resolve vision models at runtime from config. Includes docs, tests, and i18n updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant NewThread as NewThread.vue
participant ConfigPresenter as ConfigPresenter
participant ConversationManager as ConversationManager
participant SessionPresenter as SessionPresenter
User->>NewThread: open new chat
NewThread->>NewThread: initActiveModel()
alt Not ACP
NewThread->>ConfigPresenter: getSetting('defaultModel')
ConfigPresenter-->>NewThread: {providerId, modelId}
NewThread->>NewThread: resolve & set active model
NewThread->>NewThread: return early
else ACP
NewThread->>NewThread: fallback existing init flow
end
NewThread->>ConversationManager: createConversation(request)
alt no provider/model in request
ConversationManager->>ConfigPresenter: getDefaultModel()
ConfigPresenter-->>ConversationManager: {providerId, modelId}
ConversationManager->>ConversationManager: apply to defaultSettings
end
ConversationManager-->>SessionPresenter: conversation created
SessionPresenter->>ConfigPresenter: getSetting('assistantModel') for titles
SessionPresenter-->>User: display conversation
sequenceDiagram
participant App as App
participant ImageServer as ImageServer
participant ConfigPresenter as ConfigPresenter
participant LLMProvider as LLMProvider
App->>ImageServer: new ImageServer(undefined, undefined)
App->>ImageServer: request vision operation
ImageServer->>ImageServer: getEffectiveModel()
ImageServer->>ConfigPresenter: getDefaultVisionModel()
ConfigPresenter-->>ImageServer: {providerId, modelId}
ImageServer->>LLMProvider: call completion(resolvedProvider, resolvedModel)
LLMProvider-->>ImageServer: result
ImageServer-->>App: vision result
sequenceDiagram
participant UI as DefaultModelSettingsSection
participant ModelStore as ModelStore
participant ConfigPresenter as ConfigPresenter
UI->>UI: mounted() -> syncModelSelections()
par sync selections
UI->>ConfigPresenter: getSetting('assistantModel')
ConfigPresenter-->>UI: assistantModel
UI->>ModelStore: find matching assistant model
UI->>UI: set selectedAssistantModel
and
UI->>ConfigPresenter: getSetting('defaultModel')
ConfigPresenter-->>UI: defaultModel
UI->>ModelStore: find matching chat model
UI->>UI: set selectedChatModel
and
UI->>ConfigPresenter: getSetting('defaultVisionModel')
ConfigPresenter-->>UI: defaultVisionModel
UI->>ModelStore: filter vision-capable models
UI->>UI: set selectedVisionModel
end
User->>UI: selects model
UI->>ConfigPresenter: setSetting('defaultModel' or 'defaultVisionModel')
ConfigPresenter->>ConfigPresenter: persist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/mcpPresenter/inMemoryServers/imageServer.ts (1)
49-87:⚠️ Potential issue | 🟠 MajorDefault vision model is effectively hard-coded and won’t re-resolve.
The constructor pre-fills
provider/modelwithopenai/gpt-4o, sogetEffectiveModel()never re-readsdefaultVisionModeland never throws when it’s missing. This breaks runtime updates and the “missing default should error” requirement.🛠️ Suggested fix: only set provider/model when explicitly passed
- private provider: string - private model: string + private provider?: string + private model?: string - constructor(provider?: string, model?: string) { - const defaultVisionModel = presenter.configPresenter.getDefaultVisionModel() - this.provider = provider || defaultVisionModel?.providerId || 'openai' - this.model = model || defaultVisionModel?.modelId || 'gpt-4o' + constructor(provider?: string, model?: string) { + this.provider = provider + this.model = model this.server = new Server(
🤖 Fix all issues with AI agents
In `@src/renderer/settings/components/common/DefaultModelSettingsSection.vue`:
- Around line 97-103: The Vision ModelSelect inside
DefaultModelSettingsSection.vue is missing the exclude-providers prop, causing
ACP providers to be shown inconsistently; update the <ModelSelect> instance (the
one with :type="[ModelType.Chat, ModelType.ImageGeneration]" and
`@update`:model="handleVisionModelSelect") to add :exclude-providers="['acp']" so
it matches the other model selects and the syncModelSelections ACP filtering
logic.
- Around line 223-273: syncModelSelections lacks error handling and can produce
unhandled promise rejections; wrap the try/finally in a try/catch/finally (or
add a catch) to log errors and prevent crashes when configPresenter.getSetting
or persistModelSetting fail. Specifically, in the syncModelSelections function
catch exceptions from await calls (getSetting and persistModelSetting), use the
existing logging mechanism (or console.error/processLogger) to record the error
along with context (e.g., which setting failed), and then continue to the
finally block that resets isSyncingModelDefaults; ensure
selectedChatModel/selectedAssistantModel/selectedVisionModel assignment remains
inside the try so partial state isn't left inconsistent on error.
In `@src/renderer/src/components/mcp-config/mcpServerForm.vue`:
- Around line 62-64: The comment above the image model refs ("// imageServer
展示用（只读，来源于 defaultVisionModel）") is not in English; update it to an English
comment describing the same intent (e.g., that these are read-only display
values sourced from defaultVisionModel) near the declarations of
selectedImageModelName and selectedImageModelProvider in mcpServerForm.vue so
the repository comment/linting policy is satisfied.
- Around line 624-631: Replace the Chinese comment above the watch block with an
English comment conveying the same meaning; specifically change "imageServer
仅展示默认视觉模型，不再通过 args 配置" to something like "imageServer only displays the default
vision model and is no longer configured via args" and ensure this English
comment sits immediately above the watch([() => name.value, () => type.value, ()
=> modelStore.enabledModels], ... ) that calls
refreshImageServerDefaultModelDisplay().
- Around line 91-115: The async call inside
refreshImageServerDefaultModelDisplay to configPresenter.getSetting can reject
and currently bubbles out of the watch; wrap the await in a try/catch around
configPresenter.getSetting (and the subsequent resolution logic) to catch
errors, set selectedImageModelName.value and selectedImageModelProvider.value to
'' on error, and optionally log the error; ensure the rest of the function still
returns early when isImageServer.value is false or the defaultVisionModel is
missing, and keep usage of modelStore.enabledModels / providerEntry /
resolvedModel unchanged.
In `@src/renderer/src/components/NewThread.vue`:
- Around line 320-321: Translate the non-English inline comment above the
currentMode check in NewThread.vue into English; replace the comment "// 0) 非
ACP 模式下，优先使用 defaultModel" with an English equivalent such as "// 0) When not in
ACP mode, prefer using defaultModel" so all comments comply with the
English-only guideline and refer to the currentMode !== 'acp agent' conditional.
- Around line 323-332: The code sets active model from stored defaultModel
without checking ACP mode; update the block around
defaultModel/findEnabledModel/setActiveFromEnabled to verify the provider's ACP
status matches the app's current ACP mode before applying it: after obtaining
match = findEnabledModel(...), check the provider/model ACP flag (e.g.,
match.provider.isAcp or match.model.isAcp) against the current ACP mode (use the
existing app/state helper like isAcpMode() or equivalent), and only call
setActiveFromEnabled(...) and mark initialized.value = true when they match; if
they don't match, skip applying the defaultModel so chat mode and model remain
in sync.
🧹 Nitpick comments (5)
src/main/presenter/agentPresenter/utility/utilityHandler.ts (1)
268-305: Well-structured fallback mechanism with clear logging.The approach of preferring the
assistantModelconfig, falling back to the conversation's model on failure, and short-circuiting when both are the same is clean and correct. The warning log captures all the relevant context for debugging.One minor nit:
messagesWithLength.map((item) => item.formattedMessage)is computed identically on both lines 280 and 301. Extracting it to a local variable avoids the duplication and makes the intent clearer.♻️ Extract the mapped messages to a variable
const preferredModelId = assistantModel?.modelId || fallbackModelId + const formattedMessages = messagesWithLength.map((item) => item.formattedMessage) + let title: string try { title = await this.ctx.llmProviderPresenter.summaryTitles( - messagesWithLength.map((item) => item.formattedMessage), + formattedMessages, preferredProviderId, preferredModelId ) } catch (error) { const shouldFallback = preferredProviderId !== fallbackProviderId || preferredModelId !== fallbackModelId if (!shouldFallback) { throw error } console.warn( '[UtilityHandler] Failed to generate title with assistant model, fallback to conversation model', { preferredProviderId, preferredModelId, fallbackProviderId, fallbackModelId, error } ) title = await this.ctx.llmProviderPresenter.summaryTitles( - messagesWithLength.map((item) => item.formattedMessage), + formattedMessages, fallbackProviderId, fallbackModelId ) }docs/specs/default-model-settings/tasks.md (1)
9-57: Consider updating checklist status to match what’s already implemented.
If this PR completes items in T1–T6, marking them as done will keep the spec artifacts aligned with reality.test/main/presenter/configPresenter/defaultModelSettings.test.ts (1)
21-69: These tests don’t cover the actual ConfigPresenter methods.
They currently assert the mock behavior, not the getDefaultModel/setDefaultModel wrappers. Consider instantiating the presenter (or importing the real functions) and asserting they delegate to getSetting/setSetting.src/main/presenter/sessionPresenter/index.ts (1)
263-299: Well-structured fallback with a minor robustness concern.The preferred-then-fallback pattern is clean. One subtle edge case: lines 268–269 resolve
providerIdandmodelIdindependently via||, so ifassistantModelhas one field set but the other empty/falsy, you'd get a cross-provider mismatch (e.g., provider from config + modelId from conversation). In practice this shouldn't happen since the settings UI always persists both fields atomically, but a defensive guard would be safer.🛡️ Optional: validate assistantModel atomically
const assistantModel = this.configPresenter.getSetting<{ providerId: string; modelId: string }>( 'assistantModel' ) const fallbackProviderId = conversation.settings.providerId const fallbackModelId = conversation.settings.modelId - const preferredProviderId = assistantModel?.providerId || fallbackProviderId - const preferredModelId = assistantModel?.modelId || fallbackModelId + const hasValidAssistantModel = Boolean(assistantModel?.providerId && assistantModel?.modelId) + const preferredProviderId = hasValidAssistantModel ? assistantModel!.providerId : fallbackProviderId + const preferredModelId = hasValidAssistantModel ? assistantModel!.modelId : fallbackModelIdsrc/renderer/settings/components/common/DefaultModelSettingsSection.vue (1)
196-221: Consider extracting the setting key constants.The config keys
'assistantModel','defaultModel', and'defaultVisionModel'are repeated across the handlers andpersistModelSetting. Extracting them into constants would reduce the risk of typos and improve maintainability.
| <PopoverContent class="w-[320px] p-0" align="end"> | ||
| <ModelSelect | ||
| :type="[ModelType.Chat, ModelType.ImageGeneration]" | ||
| :vision-only="true" | ||
| @update:model="handleVisionModelSelect" | ||
| /> | ||
| </PopoverContent> |
There was a problem hiding this comment.
Vision model selector is missing :exclude-providers="['acp']".
The assistant and chat model selects both explicitly exclude 'acp', and syncModelSelections (line 252/258) also explicitly filters out 'acp' for vision. For consistency and to avoid showing ACP models in edge cases, add the same exclusion here.
Proposed fix
<ModelSelect
:type="[ModelType.Chat, ModelType.ImageGeneration]"
:vision-only="true"
+ :exclude-providers="['acp']"
`@update`:model="handleVisionModelSelect"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PopoverContent class="w-[320px] p-0" align="end"> | |
| <ModelSelect | |
| :type="[ModelType.Chat, ModelType.ImageGeneration]" | |
| :vision-only="true" | |
| @update:model="handleVisionModelSelect" | |
| /> | |
| </PopoverContent> | |
| <PopoverContent class="w-[320px] p-0" align="end"> | |
| <ModelSelect | |
| :type="[ModelType.Chat, ModelType.ImageGeneration]" | |
| :vision-only="true" | |
| :exclude-providers="['acp']" | |
| `@update`:model="handleVisionModelSelect" | |
| /> | |
| </PopoverContent> |
🤖 Prompt for AI Agents
In `@src/renderer/settings/components/common/DefaultModelSettingsSection.vue`
around lines 97 - 103, The Vision ModelSelect inside
DefaultModelSettingsSection.vue is missing the exclude-providers prop, causing
ACP providers to be shown inconsistently; update the <ModelSelect> instance (the
one with :type="[ModelType.Chat, ModelType.ImageGeneration]" and
`@update`:model="handleVisionModelSelect") to add :exclude-providers="['acp']" so
it matches the other model selects and the syncModelSelections ACP filtering
logic.
| // imageServer 展示用（只读，来源于 defaultVisionModel） | ||
| const selectedImageModelName = ref('') | ||
| const selectedImageModelProvider = ref('') |
There was a problem hiding this comment.
Translate the new comment to English.
Line 62 introduces a non‑English comment, which violates the repository rule.
✅ Suggested fix
-// imageServer 展示用（只读，来源于 defaultVisionModel）
+// imageServer display (read-only, sourced from defaultVisionModel)As per coding guidelines, "All logs and comments must be in English".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // imageServer 展示用（只读，来源于 defaultVisionModel） | |
| const selectedImageModelName = ref('') | |
| const selectedImageModelProvider = ref('') | |
| // imageServer display (read-only, sourced from defaultVisionModel) | |
| const selectedImageModelName = ref('') | |
| const selectedImageModelProvider = ref('') |
🤖 Prompt for AI Agents
In `@src/renderer/src/components/mcp-config/mcpServerForm.vue` around lines 62 -
64, The comment above the image model refs ("// imageServer 展示用（只读，来源于
defaultVisionModel）") is not in English; update it to an English comment
describing the same intent (e.g., that these are read-only display values
sourced from defaultVisionModel) near the declarations of selectedImageModelName
and selectedImageModelProvider in mcpServerForm.vue so the repository
comment/linting policy is satisfied.
| const refreshImageServerDefaultModelDisplay = async (): Promise<void> => { | ||
| if (!isImageServer.value) { | ||
| selectedImageModelName.value = '' | ||
| selectedImageModelProvider.value = '' | ||
| return | ||
| } | ||
|
|
||
| const defaultVisionModel = (await configPresenter.getSetting('defaultVisionModel')) as | ||
| | { providerId: string; modelId: string } | ||
| | undefined | ||
| if (!defaultVisionModel?.providerId || !defaultVisionModel?.modelId) { | ||
| selectedImageModelName.value = '' | ||
| selectedImageModelProvider.value = '' | ||
| return | ||
| } | ||
|
|
||
| selectedImageModelProvider.value = defaultVisionModel.providerId | ||
| const providerEntry = modelStore.enabledModels.find( | ||
| (entry) => entry.providerId === defaultVisionModel.providerId | ||
| ) | ||
| const resolvedModel = providerEntry?.models.find( | ||
| (model) => model.id === defaultVisionModel.modelId | ||
| ) | ||
| selectedImageModelName.value = | ||
| resolvedModel?.name || `${defaultVisionModel.providerId}/${defaultVisionModel.modelId}` |
There was a problem hiding this comment.
Guard async presenter call to avoid unhandled rejections.
configPresenter.getSetting can fail; right now a rejection bubbles into a watch callback.
✅ Suggested fix
const refreshImageServerDefaultModelDisplay = async (): Promise<void> => {
if (!isImageServer.value) {
selectedImageModelName.value = ''
selectedImageModelProvider.value = ''
return
}
- const defaultVisionModel = (await configPresenter.getSetting('defaultVisionModel')) as
- | { providerId: string; modelId: string }
- | undefined
- if (!defaultVisionModel?.providerId || !defaultVisionModel?.modelId) {
- selectedImageModelName.value = ''
- selectedImageModelProvider.value = ''
- return
- }
+ try {
+ const defaultVisionModel = (await configPresenter.getSetting('defaultVisionModel')) as
+ | { providerId: string; modelId: string }
+ | undefined
+ if (!defaultVisionModel?.providerId || !defaultVisionModel?.modelId) {
+ selectedImageModelName.value = ''
+ selectedImageModelProvider.value = ''
+ return
+ }
- selectedImageModelProvider.value = defaultVisionModel.providerId
- const providerEntry = modelStore.enabledModels.find(
- (entry) => entry.providerId === defaultVisionModel.providerId
- )
- const resolvedModel = providerEntry?.models.find(
- (model) => model.id === defaultVisionModel.modelId
- )
- selectedImageModelName.value =
- resolvedModel?.name || `${defaultVisionModel.providerId}/${defaultVisionModel.modelId}`
+ selectedImageModelProvider.value = defaultVisionModel.providerId
+ const providerEntry = modelStore.enabledModels.find(
+ (entry) => entry.providerId === defaultVisionModel.providerId
+ )
+ const resolvedModel = providerEntry?.models.find(
+ (model) => model.id === defaultVisionModel.modelId
+ )
+ selectedImageModelName.value =
+ resolvedModel?.name || `${defaultVisionModel.providerId}/${defaultVisionModel.modelId}`
+ } catch (error) {
+ console.warn('Failed to load default vision model:', error)
+ selectedImageModelName.value = ''
+ selectedImageModelProvider.value = ''
+ }
}🤖 Prompt for AI Agents
In `@src/renderer/src/components/mcp-config/mcpServerForm.vue` around lines 91 -
115, The async call inside refreshImageServerDefaultModelDisplay to
configPresenter.getSetting can reject and currently bubbles out of the watch;
wrap the await in a try/catch around configPresenter.getSetting (and the
subsequent resolution logic) to catch errors, set selectedImageModelName.value
and selectedImageModelProvider.value to '' on error, and optionally log the
error; ensure the rest of the function still returns early when
isImageServer.value is false or the defaultVisionModel is missing, and keep
usage of modelStore.enabledModels / providerEntry / resolvedModel unchanged.
| // 0) 非 ACP 模式下，优先使用 defaultModel | ||
| if (currentMode !== 'acp agent') { |
There was a problem hiding this comment.
Translate the new comment to English.
Line 320 introduces a non‑English comment.
✅ Suggested fix
- // 0) 非 ACP 模式下，优先使用 defaultModel
+ // 0) Prefer defaultModel when not in ACP modeAs per coding guidelines, "All logs and comments must be in English".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 0) 非 ACP 模式下，优先使用 defaultModel | |
| if (currentMode !== 'acp agent') { | |
| // 0) Prefer defaultModel when not in ACP mode | |
| if (currentMode !== 'acp agent') { |
🤖 Prompt for AI Agents
In `@src/renderer/src/components/NewThread.vue` around lines 320 - 321, Translate
the non-English inline comment above the currentMode check in NewThread.vue into
English; replace the comment "// 0) 非 ACP 模式下，优先使用 defaultModel" with an English
equivalent such as "// 0) When not in ACP mode, prefer using defaultModel" so
all comments comply with the English-only guideline and refer to the currentMode
!== 'acp agent' conditional.
…handling - Add ACP mode check when applying default model in NewThread.vue - Add error handling in DefaultModelSettingsSection.vue syncModelSelections - Update imageModel i18n keys to remove 'select' verb for display-only fields
* docs: add spec for default model setting * feat: #1174 support default models setting * fix(settings): sync model selections with ACP mode and improve error handling - Add ACP mode check when applying default model in NewThread.vue - Add error handling in DefaultModelSettingsSection.vue syncModelSelections - Update imageModel i18n keys to remove 'select' verb for display-only fields
* docs: add spec for default model setting * feat: #1174 support default models setting * fix(settings): sync model selections with ACP mode and improve error handling - Add ACP mode check when applying default model in NewThread.vue - Add error handling in DefaultModelSettingsSection.vue syncModelSelections - Update imageModel i18n keys to remove 'select' verb for display-only fields
1 participant
finish #1174
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation
Tests
Localization