refactor(mcp): simplify MCP config and merge market into settings#1329
Merged
refactor(mcp): simplify MCP config and merge market into settings#1329
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors MCP from a "default servers" model to per-server Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer (UI)
participant Store as Mcp Store
participant Presenter as ConfigPresenter
participant Mcp as McpPresenter
participant Manager as ServerManager
UI->>Store: toggleServer(name)
Store->>Presenter: setMcpServerEnabled(name, true)
Presenter->>Mcp: setMcpServerEnabled(name, true)
Mcp->>Presenter: persist config (setMcpServers)
Mcp->>Manager: startServer(name)
Manager-->>Mcp: started / error
Mcp-->>Presenter: on start success/failure (may rollback enabled)
Presenter-->>Store: notify config change (getEnabledMcpServers)
Store-->>UI: update enabledServers / UI state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
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 (3)
src/renderer/settings/main.ts (1)
22-149:⚠️ Potential issue | 🔴 CriticalRemove or redirect the
settings-mcp-marketi18n key and route reference.The router no longer defines a
settings-mcp-marketroute, but the i18n key is still present across all 12 language files and actively used insrc/renderer/src/components/mcp-config/components/McpServers.vue(lines 325, 330) to display a button. This creates a broken UI state where the button exists but has no destination. Either preserve a compatibility route that redirects to the MCP settings view, or remove the i18n entries and update the component to remove the button entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/main.ts` around lines 22 - 149, The i18n key and button for "settings-mcp-market" are stale because the router no longer has a settings-mcp-market route; update McpServers.vue (the component that renders the button around lines referenced) to either remove the button and any usage of the "settings-mcp-market" i18n key, or wire the button to an existing route by replacing its navigation target with the current settings-mcp route (name: "settings-mcp") or programmatically redirect to the settings MCP view; also remove the "settings-mcp-market" entries from the language files if you remove the button to keep translations in sync.src/main/presenter/syncPresenter/index.ts (1)
798-820:⚠️ Potential issue | 🟡 MinorPersist the legacy
defaultServerscleanup.If the target
mcp-settings.jsonalready exists and the only migration step here is deletingdefaultServers,settingsChangedstays false and this branch skips the write. That leaves the legacy field on disk after import, so old backups are only partially migrated.Suggested fix
const currentSettings = this.readMcpSettings(targetPath) || {} + const hadLegacyDefaultServers = Object.prototype.hasOwnProperty.call( + currentSettings, + 'defaultServers' + ) const mergedServers: Record<string, MCPServerConfig> = currentSettings.mcpServers ? { ...currentSettings.mcpServers } : {} @@ const mergedSettings: McpSettings = { ...currentSettings } mergedSettings.mcpServers = mergedServers delete mergedSettings.defaultServers + const removedLegacyDefaultServers = hadLegacyDefaultServers let settingsChanged = false @@ - if (addedServers || enabledChanged || settingsChanged) { + if (addedServers || enabledChanged || settingsChanged || removedLegacyDefaultServers) { fs.writeFileSync(targetPath, JSON.stringify(mergedSettings, null, 2), 'utf-8') return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/syncPresenter/index.ts` around lines 798 - 820, The code deletes mergedSettings.defaultServers but doesn't mark settingsChanged, so if the file already exists and only change is removing defaultServers the write is skipped and legacy field remains on disk; update the logic (in the block around mergedSettings, delete mergedSettings.defaultServers, settingsChanged) to detect if defaultServers existed (e.g., check if currentSettings.defaultServers !== undefined or if 'defaultServers' in mergedSettings before deletion) and set settingsChanged = true when it did so, ensuring the file is rewritten to persist the removal; keep references to mergedSettings, defaultServers, settingsChanged, targetPath, addedServers, and enabledChanged when making the change.src/main/presenter/mcpPresenter/index.ts (1)
124-170:⚠️ Potential issue | 🟠 MajorRespect the global MCP toggle during startup.
initialize()now boots every enabled server unconditionally. If the user disabled MCP before restart, those servers still come up on the next launch and emitSERVER_STARTED, which breaks the persisted off state.Suggested fix
- const [servers, enabledServers] = await Promise.all([ + const [servers, enabledServers, mcpEnabled] = await Promise.all([ this.configPresenter.getMcpServers(), - this.configPresenter.getEnabledMcpServers() + this.configPresenter.getEnabledMcpServers(), + this.configPresenter.getMcpEnabled() ]) @@ - if (enabledServers.length > 0) { + if (mcpEnabled && enabledServers.length > 0) { for (const serverName of enabledServers) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/index.ts` around lines 124 - 170, The initialize flow currently starts all enabled MCP servers regardless of the global MCP toggle; update initialize() to check the global toggle (e.g. this.configPresenter.isMcpEnabled() or equivalent getter) before attempting to start any servers (both the customPromptsServerName block and the enabledServers loop), and if MCP is globally disabled simply skip starting servers and avoid emitting MCP_EVENTS.SERVER_STARTED; gate the startServer calls and eventBus.send calls behind this single boolean check so persisted "off" state is respected.
🧹 Nitpick comments (8)
src/renderer/src/i18n/he-IL/settings.json (1)
723-725: Keep the new i18n path segment lowercase.
marketMenuintroduces a camelCase segment in a newly added translation path. Please align new keys with the repo’s lowercase hierarchical convention before this spreads across locales and lookups.As per coding guidelines "Use dot-separated hierarchical structure for translation key naming with lowercase letters and descriptive names grouped by feature/context (e.g., common.button.submit, chat.send.placeholder)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/he-IL/settings.json` around lines 723 - 725, The translation key "marketMenu" uses camelCase; rename this JSON object key to a lowercase segment (e.g., "marketmenu" or another all-lowercase name consistent with your repo convention) and update any lookups/usages that reference marketMenu to the new all-lowercase key, ensuring the nested entry for the "higress" value remains unchanged.src/renderer/src/i18n/zh-TW/chat.json (1)
31-35: Avoid camelCase in the new translation path.
openSettingsbreaks the lowercase hierarchical naming rule for newly added i18n keys. Please keep the new MCP block on the repo’s lowercase convention as well.As per coding guidelines "Use dot-separated hierarchical structure for translation key naming with lowercase letters and descriptive names grouped by feature/context (e.g., common.button.submit, chat.send.placeholder)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-TW/chat.json` around lines 31 - 35, The key "openSettings" in the "mcp" translation block uses camelCase and violates the lowercase hierarchical naming convention; rename it to a lowercase, hierarchical name (either split into a nested object like mcp.open.settings with keys "open": { "settings": "打開 MCP 設置" } or a single lowercase key like "open_settings") and update any code lookups that reference mcp.openSettings to the new key (references: the "mcp" translation block and any usages looking up "openSettings").src/shared/types/presenters/legacy.presenters.d.ts (1)
1463-1470: Keepenabledoptional on the shared MCP config shape.The runtime still accepts configs without this field (
normalizeServerConfig()backfills it, and the deeplink installer already has to cast around it), so making it required here turns a supported legacy/import payload into a type error and pushes more unsafe casts into the codebase. Either keepenabled?: booleanon the wire/storage type or split raw vs normalized MCP config types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/legacy.presenters.d.ts` around lines 1463 - 1470, MCPServerConfig currently requires enabled which breaks callers that pass raw/legacy payloads; make enabled optional on the shared wire/storage shape (change enabled: boolean to enabled?: boolean) or alternatively introduce two types (RawMCPServerConfig and NormalizedMCPServerConfig) and use normalizeServerConfig() to produce the normalized type; update any usages that assume required enabled (including places that cast) to either call normalizeServerConfig() or narrow to the normalized type.test/main/presenter/acpMcpPassthrough.test.ts (1)
59-117: Add a disabled-server passthrough case.This test now carries the
enabledflag, but it still only asserts theenabled: truepath. A sibling case with a selectedenabled: falseserver would lock in the new semantics and catch ACP forwarding disabled MCPs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/acpMcpPassthrough.test.ts` around lines 59 - 117, Add a sibling test that verifies disabled MCP servers are not forwarded: create a new it(...) similar to "passes only compatible selected MCP servers to newSession" but include a selected server entry with enabled: false in the mock getMcpServers return and keep it present in getAgentMcpSelections; instantiate AcpSessionManager and call initializeSession (same as existing test) and assert that handle.connection.newSession is called with mcpServers that do NOT include the disabled server. Reference the same symbols: AcpSessionManager, initializeSession, configPresenter.getAgentMcpSelections, configPresenter.getMcpServers, and handle.connection.newSession to locate where to add the assertion.src/renderer/settings/components/McpBuiltinMarket.vue (1)
140-151: Extract the new props/emits contract into interfaces.This works, but the new embedded-mode API is typed inline. Pulling these into named TypeScript interfaces keeps the component aligned with the repo’s Vue convention and makes the contract easier to reuse/read. As per coding guidelines "Define props and emits explicitly in Vue components using
definePropsanddefineEmitswith TypeScript interfaces".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/McpBuiltinMarket.vue` around lines 140 - 151, Extract the inline typing into named TypeScript interfaces: create an interface (e.g., McpBuiltinMarketProps) that declares embedded?: boolean and use it with withDefaults(defineProps<McpBuiltinMarketProps>(), { embedded: false }), and create an emits type/interface (e.g., McpBuiltinMarketEmits or type McpBuiltinMarketEmit) that defines the back event and use it with defineEmits<McpBuiltinMarketEmits>(); ensure the new interfaces are exported or declared near the top of the component so other code can reuse them and update any references to the inline types to use these new names.src/renderer/src/i18n/en-US/settings.json (1)
723-725: Prefer a lowercase hierarchical key for the new locale entry.
marketMenuadds a new camelCase branch to the locale tree. For new keys, align this with the repo convention and use a lowercase hierarchical path instead (for example,market.menu.higress), then mirror that name in the callers and sibling locales. As per coding guidelines "Use dot-separated hierarchical structure for translation key naming with lowercase letters and descriptive names grouped by feature/context" and "Use English (en-US) as the reference for translation accuracy when adding new keys".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/en-US/settings.json` around lines 723 - 725, The locale uses a camelCase branch "marketMenu" which breaks the repo convention; rename the key to a lowercase dot-separated path (e.g., change "marketMenu": { "higress": "Higress" } to "market.menu.higress": "Higress") and update every caller that references the old key (search for usages of "marketMenu" and replace with "market.menu.higress"); also mirror this new key name in sibling locale files to keep translations aligned and consistent with en-US.test/main/presenter/SyncPresenter.test.ts (1)
117-123: Split legacy-migration and new-schema MCP fixtures.This fixture sets
imported.enabledtofalsebut also includesdefaultServers: ['imported'], then asserts that the imported server ends upenabled: true. That makes the test depend on implicit precedence between the old and new schemas. I’d split this into one migration test fordefaultServersand one pureenabled-only backup test so future cleanup doesn’t create false failures.Also applies to: 175-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/SyncPresenter.test.ts` around lines 117 - 123, The mcpSettings test fixture mixes legacy "defaultServers" migration behavior with the new per-server "enabled" flag (mcpServers.imported.enabled = false plus defaultServers: ['imported']) which creates implicit precedence; split into two tests: one migration-focused test that uses defaultServers: ['imported'] with no per-server enabled flag to assert that the migration sets imported.enabled to true, and a separate enabled-flag-only test that sets mcpServers.imported.enabled explicitly (true/false) with no defaultServers to assert enabled is honored; update the fixtures/expectations in the tests that reference mcpSettings (the blocks around the current assertions at the two locations) so each test exercises only one behavior (migration OR explicit enabled) and remove the mixed-case fixture from either test.src/renderer/src/components/chat-input/McpIndicator.vue (1)
91-103: Consider improving the settings window navigation reliability.The duplicate
navigateToMcpSettingscall with a 250ms timeout appears to be a workaround for a race condition where the settings window might not be ready to receive navigation events. This approach is fragile:
- The 250ms delay is arbitrary and may be insufficient on slower machines
- The condition
getSettingsWindowId() === settingsWindowIddoesn't guarantee the window is ready to receive eventsConsider using an event-based approach where the settings window signals readiness, or checking if
createSettingsWindowreturns a promise that resolves when the window is ready.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/chat-input/McpIndicator.vue` around lines 91 - 103, openSettings currently calls navigateToMcpSettings twice with an arbitrary 250ms timeout which is fragile; change it to wait for an explicit readiness signal from the settings window instead of using setTimeout: have windowPresenter.createSettingsWindow either resolve when the settings window is ready or emit an IPC/event (e.g. "settings-window-ready") that you can listen for, then call navigateToMcpSettings(settingsWindowId) only after that readiness signal (using windowPresenter.getSettingsWindowId() to ensure the id matches), and finally set panelOpen.value = false; update openSettings to remove the fixed timeout and duplicate call and rely on the readiness promise/event from createSettingsWindow or an IPC listener to make navigation deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/mcpConfHelper.ts`:
- Around line 922-928: When adding the Apple system integration entry in the
block that currently checks isMacOS() and mcpServers, also consult
removedBuiltInServers to avoid re-creating a built-in the user removed: before
assigning mcpServers['deepchat/apple-server'] (and setting hasChanges), check
that removedBuiltInServers does not include 'deepchat/apple-server'; only add
the PLATFORM_SPECIFIC_SERVERS['deepchat/apple-server'] config and set enabled
when the entry is absent from both mcpServers and removedBuiltInServers.
- Around line 366-378: The normalizeServerConfig currently falls back to
defaultEnabledServers when legacyEnabledServers is empty, which re-enables
servers users explicitly opted out of; change normalizeServerConfig(serverName,
config, legacyEnabledServers, defaultEnabledServers) to accept an additional
boolean (e.g. legacyKeysPresent) or otherwise detect presence of legacy keys,
and update the enabled logic to: if typeof config.enabled === 'boolean' use it;
else if legacyKeysPresent use legacyEnabledServers.has(serverName) (so an empty
legacy set is honored and prevents falling back); otherwise use
defaultEnabledServers.has(serverName). Update all callers of
normalizeServerConfig to pass the new legacyKeysPresent flag.
In `@src/main/presenter/mcpPresenter/index.ts`:
- Around line 369-370: The method setMcpServerEnabled currently only updates
config via configPresenter.setMcpServerEnabled(serverName, enabled) and must
also apply the change to the running MCP runtime; after the config update,
detect whether MCP is currently active (e.g. check this.isMcpEnabled() or
this.mcpRuntime?.isRunning()) and then call the runtime control to start or stop
that specific server (e.g. this.mcpRuntime.startServer(serverName) when enabled
is true, or this.mcpRuntime.stopServer(serverName) when enabled is false); if
your runtime API differs, invoke the equivalent per-server start/stop method on
the in-memory MCP controller after the config change so enabling/disabling takes
effect immediately.
- Around line 1151-1170: When re-enabling MCP the code only starts servers from
getEnabledMcpServers() but omits the custom-prompts bootstrap (which
initialize() normally starts), so update the enable branch to also start the
custom prompts server; after iterating enabledServers call
this.startServer('deepchat-inmemory/custom-prompts-server') (with try/catch and
logging similar to the existing loop) or invoke the same bootstrap routine used
in initialize(), ensuring you reference startServer and initialize to mirror the
original bootstrap behavior.
In `@src/renderer/src/i18n/en-US/mcp.json`:
- Line 70: Update the "enabledDescription" value to avoid "start automatically"
wording and instead say that DeepChat will connect to services; specifically
modify the JSON key "enabledDescription" in mcp.json so the string reads
something like "When enabled, DeepChat will automatically connect to enabled
services (for remote MCPs this means establishing connections to HTTP/SSE
servers)." This keeps the key name the same and only changes the user-facing
text to accurately describe connecting vs. starting.
In `@src/renderer/src/stores/mcp.ts`:
- Around line 646-683: The catch currently only restores local Pinia state but
not the persisted config, so if setMcpServerEnabledMutation.mutateAsync already
flipped the persisted enabled flag and mcpPresenter.startServer/stopServer
throws, the persisted config remains wrong; fix by performing a compensating
persistence update in the rollback: call
setMcpServerEnabledMutation.mutateAsync([serverName, previousConfig.enabled])
(or persist the new state only after successful start/stop instead of before)
when reverting in the catch block and ensure
serverLoadingStates.value[serverName] is reset there as well.
In `@src/types/i18n.d.ts`:
- Around line 1119-1121: Add the missing "mcp" schema to the input locale type
so calls like t('input.mcp.*') are typed; update the
DefineLocaleMessage['input'] (in the i18n declaration where "marketMenu" is
defined) to include an mcp object with properties badge, title, empty, and
openSettings (all strings) so the input type matches the 12 locale files.
---
Outside diff comments:
In `@src/main/presenter/mcpPresenter/index.ts`:
- Around line 124-170: The initialize flow currently starts all enabled MCP
servers regardless of the global MCP toggle; update initialize() to check the
global toggle (e.g. this.configPresenter.isMcpEnabled() or equivalent getter)
before attempting to start any servers (both the customPromptsServerName block
and the enabledServers loop), and if MCP is globally disabled simply skip
starting servers and avoid emitting MCP_EVENTS.SERVER_STARTED; gate the
startServer calls and eventBus.send calls behind this single boolean check so
persisted "off" state is respected.
In `@src/main/presenter/syncPresenter/index.ts`:
- Around line 798-820: The code deletes mergedSettings.defaultServers but
doesn't mark settingsChanged, so if the file already exists and only change is
removing defaultServers the write is skipped and legacy field remains on disk;
update the logic (in the block around mergedSettings, delete
mergedSettings.defaultServers, settingsChanged) to detect if defaultServers
existed (e.g., check if currentSettings.defaultServers !== undefined or if
'defaultServers' in mergedSettings before deletion) and set settingsChanged =
true when it did so, ensuring the file is rewritten to persist the removal; keep
references to mergedSettings, defaultServers, settingsChanged, targetPath,
addedServers, and enabledChanged when making the change.
In `@src/renderer/settings/main.ts`:
- Around line 22-149: The i18n key and button for "settings-mcp-market" are
stale because the router no longer has a settings-mcp-market route; update
McpServers.vue (the component that renders the button around lines referenced)
to either remove the button and any usage of the "settings-mcp-market" i18n key,
or wire the button to an existing route by replacing its navigation target with
the current settings-mcp route (name: "settings-mcp") or programmatically
redirect to the settings MCP view; also remove the "settings-mcp-market" entries
from the language files if you remove the button to keep translations in sync.
---
Nitpick comments:
In `@src/renderer/settings/components/McpBuiltinMarket.vue`:
- Around line 140-151: Extract the inline typing into named TypeScript
interfaces: create an interface (e.g., McpBuiltinMarketProps) that declares
embedded?: boolean and use it with
withDefaults(defineProps<McpBuiltinMarketProps>(), { embedded: false }), and
create an emits type/interface (e.g., McpBuiltinMarketEmits or type
McpBuiltinMarketEmit) that defines the back event and use it with
defineEmits<McpBuiltinMarketEmits>(); ensure the new interfaces are exported or
declared near the top of the component so other code can reuse them and update
any references to the inline types to use these new names.
In `@src/renderer/src/components/chat-input/McpIndicator.vue`:
- Around line 91-103: openSettings currently calls navigateToMcpSettings twice
with an arbitrary 250ms timeout which is fragile; change it to wait for an
explicit readiness signal from the settings window instead of using setTimeout:
have windowPresenter.createSettingsWindow either resolve when the settings
window is ready or emit an IPC/event (e.g. "settings-window-ready") that you can
listen for, then call navigateToMcpSettings(settingsWindowId) only after that
readiness signal (using windowPresenter.getSettingsWindowId() to ensure the id
matches), and finally set panelOpen.value = false; update openSettings to remove
the fixed timeout and duplicate call and rely on the readiness promise/event
from createSettingsWindow or an IPC listener to make navigation deterministic.
In `@src/renderer/src/i18n/en-US/settings.json`:
- Around line 723-725: The locale uses a camelCase branch "marketMenu" which
breaks the repo convention; rename the key to a lowercase dot-separated path
(e.g., change "marketMenu": { "higress": "Higress" } to "market.menu.higress":
"Higress") and update every caller that references the old key (search for
usages of "marketMenu" and replace with "market.menu.higress"); also mirror this
new key name in sibling locale files to keep translations aligned and consistent
with en-US.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 723-725: The translation key "marketMenu" uses camelCase; rename
this JSON object key to a lowercase segment (e.g., "marketmenu" or another
all-lowercase name consistent with your repo convention) and update any
lookups/usages that reference marketMenu to the new all-lowercase key, ensuring
the nested entry for the "higress" value remains unchanged.
In `@src/renderer/src/i18n/zh-TW/chat.json`:
- Around line 31-35: The key "openSettings" in the "mcp" translation block uses
camelCase and violates the lowercase hierarchical naming convention; rename it
to a lowercase, hierarchical name (either split into a nested object like
mcp.open.settings with keys "open": { "settings": "打開 MCP 設置" } or a single
lowercase key like "open_settings") and update any code lookups that reference
mcp.openSettings to the new key (references: the "mcp" translation block and any
usages looking up "openSettings").
In `@src/shared/types/presenters/legacy.presenters.d.ts`:
- Around line 1463-1470: MCPServerConfig currently requires enabled which breaks
callers that pass raw/legacy payloads; make enabled optional on the shared
wire/storage shape (change enabled: boolean to enabled?: boolean) or
alternatively introduce two types (RawMCPServerConfig and
NormalizedMCPServerConfig) and use normalizeServerConfig() to produce the
normalized type; update any usages that assume required enabled (including
places that cast) to either call normalizeServerConfig() or narrow to the
normalized type.
In `@test/main/presenter/acpMcpPassthrough.test.ts`:
- Around line 59-117: Add a sibling test that verifies disabled MCP servers are
not forwarded: create a new it(...) similar to "passes only compatible selected
MCP servers to newSession" but include a selected server entry with enabled:
false in the mock getMcpServers return and keep it present in
getAgentMcpSelections; instantiate AcpSessionManager and call initializeSession
(same as existing test) and assert that handle.connection.newSession is called
with mcpServers that do NOT include the disabled server. Reference the same
symbols: AcpSessionManager, initializeSession,
configPresenter.getAgentMcpSelections, configPresenter.getMcpServers, and
handle.connection.newSession to locate where to add the assertion.
In `@test/main/presenter/SyncPresenter.test.ts`:
- Around line 117-123: The mcpSettings test fixture mixes legacy
"defaultServers" migration behavior with the new per-server "enabled" flag
(mcpServers.imported.enabled = false plus defaultServers: ['imported']) which
creates implicit precedence; split into two tests: one migration-focused test
that uses defaultServers: ['imported'] with no per-server enabled flag to assert
that the migration sets imported.enabled to true, and a separate
enabled-flag-only test that sets mcpServers.imported.enabled explicitly
(true/false) with no defaultServers to assert enabled is honored; update the
fixtures/expectations in the tests that reference mcpSettings (the blocks around
the current assertions at the two locations) so each test exercises only one
behavior (migration OR explicit enabled) and remove the mixed-case fixture from
either test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81f3f145-fc73-4c01-b850-1dcf7e7725d2
⛔ Files ignored due to path filters (2)
docs/images/zai.en.pngis excluded by!**/*.pngdocs/images/zai.zh.pngis excluded by!**/*.png
📒 Files selected for processing (69)
README.jp.mdREADME.mdREADME.zh.mdresources/model-db/providers.jsonsrc/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/mcpConfHelper.tssrc/main/presenter/deeplinkPresenter/index.tssrc/main/presenter/llmProviderPresenter/providers/modelscopeProvider.tssrc/main/presenter/mcpPresenter/inMemoryServers/builder.tssrc/main/presenter/mcpPresenter/inMemoryServers/powerpackServer.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/mcprouterManager.tssrc/main/presenter/mcpPresenter/serverManager.tssrc/main/presenter/syncPresenter/index.tssrc/renderer/settings/components/McpBuiltinMarket.vuesrc/renderer/settings/components/McpSettings.vuesrc/renderer/settings/main.tssrc/renderer/src/components/chat-input/McpIndicator.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/mcp-config/components/McpServerCard.vuesrc/renderer/src/components/mcp-config/components/McpServers.vuesrc/renderer/src/components/mcp-config/const.tssrc/renderer/src/components/mcp-config/mcpServerForm.vuesrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/mcp.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/mcp.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/mcp.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/mcp.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/mcp.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/mcp.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/mcp.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/mcp.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/mcp.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/mcp.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/mcp.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/mcp.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/mcp.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/types/i18n.d.tstest/main/presenter/SyncPresenter.test.tstest/main/presenter/acpMcpPassthrough.test.tstest/renderer/components/ChatInputBox.test.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/components/McpServers.test.tstest/renderer/components/McpSettings.test.ts
💤 Files with no reviewable changes (18)
- src/renderer/src/components/mcp-config/const.ts
- src/renderer/src/i18n/da-DK/mcp.json
- src/renderer/src/i18n/ru-RU/mcp.json
- src/renderer/src/i18n/zh-TW/mcp.json
- src/renderer/src/i18n/zh-HK/mcp.json
- src/renderer/src/components/message/MessageBlockToolCall.vue
- src/renderer/src/i18n/ja-JP/mcp.json
- src/renderer/src/i18n/he-IL/mcp.json
- src/renderer/src/i18n/fr-FR/mcp.json
- README.zh.md
- src/renderer/src/i18n/fa-IR/mcp.json
- src/renderer/src/i18n/ko-KR/mcp.json
- src/main/presenter/mcpPresenter/inMemoryServers/builder.ts
- src/renderer/src/i18n/pt-BR/mcp.json
- README.md
- README.jp.md
- src/main/presenter/mcpPresenter/inMemoryServers/powerpackServer.ts
- src/main/presenter/mcpPresenter/serverManager.ts
Comment on lines
+1151
to
+1170
| if (enabled) { | ||
| const enabledServers = await this.configPresenter.getEnabledMcpServers() | ||
| for (const serverName of enabledServers) { | ||
| try { | ||
| await this.startServer(serverName) | ||
| } catch (error) { | ||
| console.error(`[MCP] Failed to start enabled server ${serverName}:`, error) | ||
| } | ||
| } | ||
| return | ||
| } | ||
|
|
||
| const runningClients = await this.serverManager.getRunningClients() | ||
| for (const client of runningClients) { | ||
| try { | ||
| await this.stopServer(client.serverName) | ||
| } catch (error) { | ||
| console.error(`[MCP] Failed to stop server ${client.serverName}:`, error) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Mirror the custom-prompts bootstrap when re-enabling MCP.
setMcpEnabled(false) stops every running client, but the enable path only restarts enabledServers. Since initialize() still separately boots deepchat-inmemory/custom-prompts-server, toggling MCP off and back on can leave custom prompts unavailable until restart.
Suggested fix
if (enabled) {
+ const customPromptsServerName = 'deepchat-inmemory/custom-prompts-server'
+ const servers = await this.configPresenter.getMcpServers()
+
+ if (servers[customPromptsServerName]) {
+ try {
+ await this.startServer(customPromptsServerName)
+ } catch (error) {
+ console.error(
+ `[MCP] Failed to start custom prompts server ${customPromptsServerName}:`,
+ error
+ )
+ }
+ }
+
const enabledServers = await this.configPresenter.getEnabledMcpServers()
for (const serverName of enabledServers) {📝 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.
Suggested change
| if (enabled) { | |
| const enabledServers = await this.configPresenter.getEnabledMcpServers() | |
| for (const serverName of enabledServers) { | |
| try { | |
| await this.startServer(serverName) | |
| } catch (error) { | |
| console.error(`[MCP] Failed to start enabled server ${serverName}:`, error) | |
| } | |
| } | |
| return | |
| } | |
| const runningClients = await this.serverManager.getRunningClients() | |
| for (const client of runningClients) { | |
| try { | |
| await this.stopServer(client.serverName) | |
| } catch (error) { | |
| console.error(`[MCP] Failed to stop server ${client.serverName}:`, error) | |
| } | |
| } | |
| if (enabled) { | |
| const customPromptsServerName = 'deepchat-inmemory/custom-prompts-server' | |
| const servers = await this.configPresenter.getMcpServers() | |
| if (servers[customPromptsServerName]) { | |
| try { | |
| await this.startServer(customPromptsServerName) | |
| } catch (error) { | |
| console.error( | |
| `[MCP] Failed to start custom prompts server ${customPromptsServerName}:`, | |
| error | |
| ) | |
| } | |
| } | |
| const enabledServers = await this.configPresenter.getEnabledMcpServers() | |
| for (const serverName of enabledServers) { | |
| try { | |
| await this.startServer(serverName) | |
| } catch (error) { | |
| console.error(`[MCP] Failed to start enabled server ${serverName}:`, error) | |
| } | |
| } | |
| return | |
| } | |
| const runningClients = await this.serverManager.getRunningClients() | |
| for (const client of runningClients) { | |
| try { | |
| await this.stopServer(client.serverName) | |
| } catch (error) { | |
| console.error(`[MCP] Failed to stop server ${client.serverName}:`, error) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/mcpPresenter/index.ts` around lines 1151 - 1170, When
re-enabling MCP the code only starts servers from getEnabledMcpServers() but
omits the custom-prompts bootstrap (which initialize() normally starts), so
update the enable branch to also start the custom prompts server; after
iterating enabledServers call
this.startServer('deepchat-inmemory/custom-prompts-server') (with try/catch and
logging similar to the existing loop) or invoke the same bootstrap routine used
in initialize(), ensuring you reference startServer and initialize to mirror the
original bootstrap behavior.
| }, | ||
| "enableToAccess": "Please enable MCP to access configuration options.", | ||
| "enabledDescription": "Enable or disable MCP functionality and tools.", | ||
| "enabledDescription": "When enabled, all enabled services start automatically.", |
Contributor
There was a problem hiding this comment.
Avoid “start automatically” wording for remote MCPs.
This toggle also covers HTTP/SSE servers, and those are connected to rather than started. Wording like “DeepChat will automatically connect to enabled services” would be more accurate here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/i18n/en-US/mcp.json` at line 70, Update the
"enabledDescription" value to avoid "start automatically" wording and instead
say that DeepChat will connect to services; specifically modify the JSON key
"enabledDescription" in mcp.json so the string reads something like "When
enabled, DeepChat will automatically connect to enabled services (for remote
MCPs this means establishing connections to HTTP/SSE servers)." This keeps the
key name the same and only changes the user-facing text to accurately describe
connecting vs. starting.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/presenter/mcpPresenter/index.ts (1)
1162-1172:⚠️ Potential issue | 🟡 MinorCustom prompts server may not restart when re-enabling MCP.
The
setMcpEnabled(true)path only starts servers fromgetEnabledMcpServers(). However, duringinitialize()(lines 138-155), thedeepchat-inmemory/custom-prompts-serveris started unconditionally if it exists. This asymmetry means toggling MCP off and back on could leave the custom prompts server unavailable until application restart.This was flagged in a previous review. If custom-prompts-server should behave as a system-managed server, consider adding it to the enable path:
Suggested fix
if (enabled) { + // Restart system-managed servers + const servers = await this.configPresenter.getMcpServers() + const customPromptsServerName = 'deepchat-inmemory/custom-prompts-server' + if (servers[customPromptsServerName]) { + try { + await this.startServer(customPromptsServerName) + } catch (error) { + console.error(`[MCP] Failed to start custom prompts server:`, error) + } + } + const enabledServers = await this.configPresenter.getEnabledMcpServers() for (const serverName of enabledServers) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/index.ts` around lines 1162 - 1172, The setMcpEnabled(true) branch currently only starts servers returned by getEnabledMcpServers(), which skips the unconditionally-started deepchat-inmemory/custom-prompts-server from initialize(); update setMcpEnabled (or the start path within it) to also ensure the custom prompts server is started when re-enabling MCP: either add "deepchat-inmemory/custom-prompts-server" to the list returned by getEnabledMcpServers() or explicitly call startServer('deepchat-inmemory/custom-prompts-server') in the enabled branch (the code that iterates enabledServers and calls startServer) so the custom prompts server is restored without requiring an app restart.
🧹 Nitpick comments (4)
src/main/presenter/mcpPresenter/index.ts (2)
376-378: Consider checking if server is already running before starting.When enabling a server,
startServeris called unconditionally. If the server is already running (edge case), this could cause issues depending on howserverManager.startServerhandles double-start scenarios.Suggested defensive check
if (enabled) { + if (!(await this.isServerRunning(serverName))) { await this.startServer(serverName) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/index.ts` around lines 376 - 378, When enabling the server, avoid unconditionally calling this.startServer(serverName); first query the server state (e.g. via a status method on your manager such as serverManager.isRunning(serverName) or serverManager.getStatus(serverName)) and only call this.startServer(serverName) if the server is not already running, and wrap the start call in a try/catch to handle race conditions or duplicate-start errors from serverManager.startServer/startServer.
379-382: Consider checking if server is running before stopping.The disable path unconditionally calls
stopServer, which will emit aSERVER_STOPPEDevent even if the server wasn't running. While this is likely harmless (stopServer should be idempotent), it may cause unnecessary event emissions.Suggested improvement
if (enabled) { await this.startServer(serverName) return } - await this.stopServer(serverName) + if (await this.isServerRunning(serverName)) { + await this.stopServer(serverName) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/index.ts` around lines 379 - 382, The code unconditionally calls this.stopServer(serverName) and can emit SERVER_STOPPED even when the server isn't running; before calling stopServer, check the server's running state (e.g. via an existing isServerRunning(serverName) helper or this.serverManager.isRunning(serverName) or by inspecting this.getServer/serverMap) and only call await this.stopServer(serverName) when that check returns true, otherwise skip calling stopServer to avoid spurious SERVER_STOPPED events.test/main/presenter/mcpPresenter.test.ts (1)
141-152: Consider adding a test for runtime failure rollback.This test validates that runtime failures reject and config was persisted. However, there's no test for whether the config should be rolled back on runtime failure. Based on the current implementation in
McpPresenter#setMcpServerEnabled, runtime failures propagate without rollback, which may leave the system in an inconsistent state (config says enabled, but server isn't running).If this is intentional (relying on next restart to sync), consider adding a comment documenting this behavior. If rollback is desired, the implementation would need adjustment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/mcpPresenter.test.ts` around lines 141 - 152, Add a test or document intended behavior: either (A) implement and test rollback by updating McpPresenter#setMcpServerEnabled to call configPresenter.setMcpServerEnabled('demo-server', false) when startServer (spied in the test via presenter.startServer) rejects and assert the rollback in the test, or (B) if no rollback is intended, add a clear comment in McpPresenter#setMcpServerEnabled explaining that runtime failures do not revert persisted config and that recovery is expected on next restart; update the test (mcpPresenter.test.ts) to reflect the chosen behavior using createConfigPresenter and presenter.startServer mocks.src/main/presenter/configPresenter/mcpConfHelper.ts (1)
448-448: Chinese comments should be translated to English.Per coding guidelines, all logs and comments must be in English for TypeScript files. Several comments in this file are in Chinese (e.g., lines 448, 484, 490, 495, 513, 520, 530, 546, 897, 927, 933, 947, 953).
Example translations
- // 检查并补充缺少的inmemory服务 + // Check and add missing inmemory services ... - // 遍历所有默认的inmemory服务，确保它们都存在 + // Iterate all default inmemory services to ensure they exist ... - // 移除不支持当前平台的服务 + // Remove services not supported on current platform🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/mcpConfHelper.ts` at line 448, Several comments in mcpConfHelper.ts are written in Chinese (for example the comment "检查并补充缺少的inmemory服务" and other nearby Chinese phrases) and must be translated to English; update every Chinese comment and any log message in this module to clear, idiomatic English (preserve technical terms like "in-memory" and intent), replace the shown Chinese strings with their English equivalents, and run the linter/formatting to ensure no stray non-English comments remain in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/presenter/mcpPresenter/index.ts`:
- Around line 1162-1172: The setMcpEnabled(true) branch currently only starts
servers returned by getEnabledMcpServers(), which skips the
unconditionally-started deepchat-inmemory/custom-prompts-server from
initialize(); update setMcpEnabled (or the start path within it) to also ensure
the custom prompts server is started when re-enabling MCP: either add
"deepchat-inmemory/custom-prompts-server" to the list returned by
getEnabledMcpServers() or explicitly call
startServer('deepchat-inmemory/custom-prompts-server') in the enabled branch
(the code that iterates enabledServers and calls startServer) so the custom
prompts server is restored without requiring an app restart.
---
Nitpick comments:
In `@src/main/presenter/configPresenter/mcpConfHelper.ts`:
- Line 448: Several comments in mcpConfHelper.ts are written in Chinese (for
example the comment "检查并补充缺少的inmemory服务" and other nearby Chinese phrases) and
must be translated to English; update every Chinese comment and any log message
in this module to clear, idiomatic English (preserve technical terms like
"in-memory" and intent), replace the shown Chinese strings with their English
equivalents, and run the linter/formatting to ensure no stray non-English
comments remain in the file.
In `@src/main/presenter/mcpPresenter/index.ts`:
- Around line 376-378: When enabling the server, avoid unconditionally calling
this.startServer(serverName); first query the server state (e.g. via a status
method on your manager such as serverManager.isRunning(serverName) or
serverManager.getStatus(serverName)) and only call this.startServer(serverName)
if the server is not already running, and wrap the start call in a try/catch to
handle race conditions or duplicate-start errors from
serverManager.startServer/startServer.
- Around line 379-382: The code unconditionally calls
this.stopServer(serverName) and can emit SERVER_STOPPED even when the server
isn't running; before calling stopServer, check the server's running state (e.g.
via an existing isServerRunning(serverName) helper or
this.serverManager.isRunning(serverName) or by inspecting
this.getServer/serverMap) and only call await this.stopServer(serverName) when
that check returns true, otherwise skip calling stopServer to avoid spurious
SERVER_STOPPED events.
In `@test/main/presenter/mcpPresenter.test.ts`:
- Around line 141-152: Add a test or document intended behavior: either (A)
implement and test rollback by updating McpPresenter#setMcpServerEnabled to call
configPresenter.setMcpServerEnabled('demo-server', false) when startServer
(spied in the test via presenter.startServer) rejects and assert the rollback in
the test, or (B) if no rollback is intended, add a clear comment in
McpPresenter#setMcpServerEnabled explaining that runtime failures do not revert
persisted config and that recovery is expected on next restart; update the test
(mcpPresenter.test.ts) to reflect the chosen behavior using
createConfigPresenter and presenter.startServer mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10a4c7cf-b8d1-4a01-9aac-f43375a46a6a
📒 Files selected for processing (7)
src/main/presenter/configPresenter/mcpConfHelper.tssrc/main/presenter/mcpPresenter/index.tssrc/renderer/src/stores/mcp.tssrc/types/i18n.d.tstest/main/presenter/configPresenter/mcpConfHelper.test.tstest/main/presenter/mcpPresenter.test.tstest/renderer/stores/mcpStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/stores/mcp.ts
- src/types/i18n.d.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR refactors the MCP configuration system, simplifying the architecture and integrating the MCP market into the settings page.
Key Changes
Removed Features
powerpackServerin-memory server (code execution/Shell command tools)Configuration Simplification
mcpConfHelper.tsto streamline MCP server configurationserverManager.tsandmcpPresenter/index.tspowerpackrelated configuration optionsUI Integration
McpBuiltinMarket.vue)McpIndicator.vue)McpSettings.vueandMcpServers.vueinteractionsData Updates
providers.json)Testing
McpServers.test.tsandMcpSettings.test.tsSyncPresenter.test.tsandacpMcpPassthrough.test.tsFile Statistics
Impact
powerpackserver will be unavailableTesting Checklist
pnpm run i18n)pnpm test:main,pnpm test:renderer)Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Removed Features
Documentation