Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request implements plugin MCP lifecycle isolation: plugin-owned MCP servers now operate independently of the global MCP enable/disable toggle, including error tracking per-server, conditional error-notification suppression, and persistent tool/resource visibility in the renderer when global MCP is disabled. ChangesPlugin MCP Lifecycle Isolation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/stores/mcp.ts (2)
314-327:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent server status clearing when MCP is disabled.
Line 316 clears all server statuses when
mcpEnabledchanges tofalseviasyncConfigFromQuery, but lines 555-559 insetMcpEnabled(false)preserve plugin-owned server statuses. This inconsistency means:
- User toggle → plugin-owned statuses preserved (correct)
- External config change → plugin-owned statuses cleared (likely incorrect)
Both code paths should preserve plugin-owned server statuses when MCP is disabled.
🔧 Proposed fix to preserve plugin-owned server statuses
} else { // MCP disabled: clear state and refresh queries to get empty results - serverStatuses.value = {} + serverStatuses.value = Object.fromEntries( + Object.entries(serverStatuses.value).filter(([serverName]) => + isPluginOwnedServerName(serverName) + ) + ) toolInputs.value = {} toolResults.value = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/mcp.ts` around lines 314 - 327, The branch in syncConfigFromQuery that handles mcpEnabled turning false currently wipes serverStatuses entirely; instead, mirror the behavior in setMcpEnabled(false) by preserving plugin-owned statuses: when mcpEnabled is false, replace serverStatuses.value with a filtered map that keeps entries whose owner indicates a plugin (match the same ownership check used in setMcpEnabled), while removing all other servers; then clear toolInputs/toolResults and refetch toolsQuery, clientsQuery, resourcesQuery and promptsQuery and handle errors as done already. Update the logic in syncConfigFromQuery to reference serverStatuses, setMcpEnabled, and the same owner predicate so both paths behave consistently.
594-609:⚠️ Potential issue | 🟠 MajorFix early return to allow tool auto-management for plugin-owned servers when global MCP is disabled.
The early return at line 57–59 prevents tool enable/disable logic (lines 61–76) from running when
mcpEnabledis false, affecting both user-owned and plugin-owned servers. However, design spec requires plugin-owned tools to remain in the agent tool list when global MCP is disabled. The check should mirror line 595:if (!config.value.mcpEnabled && !isPluginOwnedServerConfig(serverConfig)) { return }This allows plugin-owned server tools to auto-enable/disable based on server status even when global MCP is off.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/mcp.ts` around lines 594 - 609, The early return prevents plugin-owned servers from having their tools auto-managed when global MCP is disabled; update the early-return guard in the block around serverConfig so it only returns when mcp is disabled AND the server is NOT plugin-owned (i.e., change the condition to match the earlier check: if (!config.value.mcpEnabled && !isPluginOwnedServerConfig(serverConfig) { return }), then allow the subsequent calls to mcpClient.isServerRunning, serverStatuses.value assignment, loadTools/loadClients and the isRunning-based enable/disable logic to run for plugin-owned servers.
🧹 Nitpick comments (1)
docs/issues/plugin-mcp-lifecycle-isolation/plan.md (1)
24-27: 💤 Low valueConsider varying sentence structure in the test strategy section.
The static analysis tool flagged that three consecutive bullet points begin with "Add," which can make the text feel repetitive. Consider varying the phrasing for better readability.
✍️ Optional rewording suggestion
## Test Strategy -- Add main-process tests for global switch isolation and plugin start behavior. -- Add error-notification tests for plugin-owned connection and tool-list failures. -- Add renderer tests for disabled-global MCP state with plugin tools still visible. -- Add CUA settings regression coverage for MCP error state. +- Add main-process tests for global switch isolation and plugin start behavior. +- Verify error-notification suppression for plugin-owned connection and tool-list failures. +- Test renderer behavior when global MCP is disabled but plugin tools remain visible. +- Add CUA settings regression coverage for MCP error state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/issues/plugin-mcp-lifecycle-isolation/plan.md` around lines 24 - 27, In the "test strategy" bulleted list within the plan document, three consecutive bullets start with the verb "Add", making the phrasing repetitive; edit those bullets so their opening verbs vary (for example, change one to "Include", another to "Cover" or "Validate", or rephrase to passive/nominal forms like "Main-process tests for..." and "Renderer tests covering...") while preserving the same meaning about global switch isolation, error-notification/plugin-owned connection failures, renderer behavior when MCP is disabled, and CUA settings regression coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/renderer/src/stores/mcp.ts`:
- Around line 314-327: The branch in syncConfigFromQuery that handles mcpEnabled
turning false currently wipes serverStatuses entirely; instead, mirror the
behavior in setMcpEnabled(false) by preserving plugin-owned statuses: when
mcpEnabled is false, replace serverStatuses.value with a filtered map that keeps
entries whose owner indicates a plugin (match the same ownership check used in
setMcpEnabled), while removing all other servers; then clear
toolInputs/toolResults and refetch toolsQuery, clientsQuery, resourcesQuery and
promptsQuery and handle errors as done already. Update the logic in
syncConfigFromQuery to reference serverStatuses, setMcpEnabled, and the same
owner predicate so both paths behave consistently.
- Around line 594-609: The early return prevents plugin-owned servers from
having their tools auto-managed when global MCP is disabled; update the
early-return guard in the block around serverConfig so it only returns when mcp
is disabled AND the server is NOT plugin-owned (i.e., change the condition to
match the earlier check: if (!config.value.mcpEnabled &&
!isPluginOwnedServerConfig(serverConfig) { return }), then allow the subsequent
calls to mcpClient.isServerRunning, serverStatuses.value assignment,
loadTools/loadClients and the isRunning-based enable/disable logic to run for
plugin-owned servers.
---
Nitpick comments:
In `@docs/issues/plugin-mcp-lifecycle-isolation/plan.md`:
- Around line 24-27: In the "test strategy" bulleted list within the plan
document, three consecutive bullets start with the verb "Add", making the
phrasing repetitive; edit those bullets so their opening verbs vary (for
example, change one to "Include", another to "Cover" or "Validate", or rephrase
to passive/nominal forms like "Main-process tests for..." and "Renderer tests
covering...") while preserving the same meaning about global switch isolation,
error-notification/plugin-owned connection failures, renderer behavior when MCP
is disabled, and CUA settings regression coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 616daab5-0e1d-4b03-82ab-64d64eff34d6
📒 Files selected for processing (19)
docs/issues/plugin-mcp-lifecycle-isolation/plan.mddocs/issues/plugin-mcp-lifecycle-isolation/spec.mddocs/issues/plugin-mcp-lifecycle-isolation/tasks.mdplugins/cua/settings/assets/index.jssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/serverManager.tssrc/main/presenter/mcpPresenter/toolManager.tssrc/main/presenter/pluginPresenter/index.tssrc/renderer/settings/components/PluginsSettings.vuesrc/renderer/src/stores/mcp.tssrc/shared/types/plugin.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/mcpPresenter.test.tstest/main/presenter/mcpPresenter/serverManager.test.tstest/main/presenter/mcpPresenter/toolManager.test.tstest/main/presenter/pluginPresenter.test.tstest/renderer/components/McpIndicator.test.tstest/renderer/plugins/cuaSettings.test.tstest/renderer/stores/mcpStore.test.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.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes