Skip to content

Conversation

@taylorwilsdon
Copy link
Contributor

@taylorwilsdon taylorwilsdon commented Jun 2, 2025

Related GitHub Issue

Closes: #4265
Closes: #4431

Description

This PR introduces MCP server refresh (button & on save) while introducing reliable deletion for MCP server connections in the Roo Code extension. Wasn't sure if the i18n generation is something I should trigger myself or if it runs as a github action / workflow @mrubens sorry just getting up to speed here but if there's manual action there lmk (edit - did fail on missing i18n, went ahead and had gemini take a run at it - alas I only speak enough french and spanish to verify those particular ones 😂 )

Debugging why the refresh wasn't taking in the actual VSCode panel view stumped Opus and spent like $15 before I gave it up and figured it out the old fashioned way. While I don't usually like to mix feat and bugfix, this is a requirement for this to work (and any other UI updates after initialization of the widget, this actually fixes other stuff that would have reflected without restarting the extension now too)

Key points

  • refreshAllMcpServers command & UI

    • Web-view “Refresh MCP Servers” button (McpView.tsx) sends a new "refreshAllMcpServers" message.

    • webviewMessageHandler routes the message to McpHub.refreshAllConnections(), which

      • guards against concurrent refresh with isConnecting,
      • reloads both global & project mcp_settings.json,
      • tears down old connections, re-initialises, and notifies the UI,
      • shows user-visible toasts for already refreshing, in progress, and complete.
  • Deletion view update fix

    • notifyWebviewOfServerChanges() now chooses the correct ClineProvider instance before posting, ensuring that the list re-renders immediately after a delete.
  • Supporting infrastructure

    • updateServerConnections() gains manageConnectingState so internal calls (e.g., during refresh) do not erroneously block the UI.
    • New i18n strings (common.json, mcp.json).
    • Replaced deprecated react-use useEvent with native window.addEventListener to avoid double listeners during hot reloads.

Reviewer focus

  • Thread-safety around isConnecting flag.
  • Correct disposal/re-creation of file watchers.
  • Web-view post-message flow in notifyWebviewOfServerChanges() (fallback logic).
  • i18n key names / namespace consistency.

Test Procedure

  1. Manual – refresh path

    1. Start extension with at least one working MCP server.
    2. Add a new entry to mcp_settings.json (global or project).
    3. In the UI, click Refresh MCP Servers.
    4. ✅ Expect toast: “Refreshing all MCP servers…”, followed by “All MCP servers have been refreshed.”
    5. ✅ List now contains the new server without window reload.
    6. Trigger button again quickly → toast: “MCP servers are already refreshing.”
  2. Manual – delete path

    1. Delete any server via UI action.
    2. ✅ List updates immediately; toast: “Deleted MCP server: <name>”.
  3. Regression – existing flows

    • Restart, connect, enable/disable, tool toggles – no regressions (actually improved significantly)

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change
  • ♻️ Refactor
  • 💅 Style
  • 📚 Documentation
  • ⚙️ Build/CI
  • 🧹 Chore

Pre-Submission Checklist

  • Issue Linked
  • Scope
  • Self-Review
  • Code Quality
  • Testing
  • Branch Hygiene
  • Documentation Impact
  • Changeset
  • Contribution Guidelines

Screenshots / Videos

roo_mcp_after.mov
roo_mcp_before.mov

Documentation Updates

  • No documentation updates are required.
  • Yes – (If checked, describe or link docs PR)

Additional Notes

The PR purposefully keeps refreshAllConnections() self-contained to minimise surface area; future enhancements (e.g., per-server ping) can layer on top without altering UI contracts.

Get in Touch

Discord: @taylorwilsdon


Important

Introduces MCP server refresh functionality and fixes deletion view update, with infrastructure improvements and i18n updates.

  • Behavior:
    • Adds refreshAllMcpServers command in webviewMessageHandler.ts to refresh MCP servers.
    • Implements refreshAllConnections() in McpHub.ts to reload server configurations and notify UI.
    • Updates McpView.tsx to include "Refresh MCP Servers" button.
  • Deletion Fix:
    • Fixes notifyWebviewOfServerChanges() to select correct ClineProvider instance for immediate list re-render.
  • Infrastructure:
    • Adds manageConnectingState to updateServerConnections() in McpHub.ts to prevent UI blocking during refresh.
    • Replaces useEvent with window.addEventListener in ExtensionStateContext.tsx for message handling.
    • Adds i18n strings in common.json and mcp.json for new messages and UI elements.

This description was created by Ellipsis for aeac9e4. You can customize this summary. It will automatically update as commits are pushed.

@taylorwilsdon taylorwilsdon requested review from cte and mrubens as code owners June 2, 2025 22:18
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working enhancement New feature or request labels Jun 2, 2025
@taylorwilsdon taylorwilsdon changed the title feat: Refresh MCP Servers feat: Refresh MCP Servers, fix MCP server management UI display Jun 2, 2025
@taylorwilsdon taylorwilsdon changed the title feat: Refresh MCP Servers, fix MCP server management UI display feat: Add Refresh MCP Servers capability, fix state changes in MCP server management UI view Jun 2, 2025
@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 2, 2025
Comment on lines 1004 to 1006
t("common:info.mcp_servers_active", {
mcpServers: `Global: ${globalServerNames.join(", ") || "none"}`,
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few nitpicks on the internationalization that I can help out with later:

  1. I think this would probably be better in another namespace - is there something better suited than common?
  2. Ideally we can also translate "Global", "Project", and "none". I think maybe I would have a global_mcp_servers_active and project_mcp_servers_active strings in case the order is different in any languages. "none" can probably live under common .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ the i18n is always a little different project to project, figured there would be a little bit of a learning curve. Happy to expand any coverage as needed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mrubens, just circling back here to clean these up - should have some time after work tonight. I'm thinking todo items are:

  • add global_mcp_servers_active and project_mcp_servers_active in place of the general mcp_servers_active where applicable
  • refac mcp related i18n strings from common.json into mcp.json (clarifying question below)

The pattern I was following was where the existing mcp server related information messages lived ie "common:info.mcp_server_connected" based on #3865582 but I see there's also the mcp.json in each locale so I'm thinking that is probably the right home? Let me know if that's the correct landing spot more than happy to handle the refac. I can move all the mcp related ones into mcp.json unless you'd prefer otherwise!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added global and project level + i18n in current location and pushed up, will hold off until I hear otherwise as far as moving them into a different locale file. Looking at the structure, it appears you've got two different approaches:

  • /src/i18n/locales/{locale}/ with common.json & tools.json as the only options
  • /webview-ui/src/i18n/locales/{locale} with account.json chat.json common.json mcp.json etc

The existing i18n in McpHub all pull from the former source so if approved I will refac to place all of those in mcp.json in their relevant webview-ui locale

@taylorwilsdon
Copy link
Contributor Author

Failing test doesn't appear related to this change so I won't touch it here to avoid complexity

taylorwilsdon and others added 3 commits June 4, 2025 10:52
- Add flexWrap to button container to allow wrapping on smaller screens
- Change buttons from fixed flex:1 to flex: 1 1 auto with minWidth
- Ensures buttons maintain minimum readable width of 120px
- Prevents button text from being truncated on narrow viewports
@daniel-lxs daniel-lxs requested a review from jr as a code owner June 6, 2025 16:19
- Added createTextEditorDecorationType to vscode mock in McpHub.test.ts
- Created execa mock to handle ESM module import issues
- Fixes test failure that was exposed by PR RooCodeInc#4267's module loading changes
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Thank you @taylorwilsdon!

Looks good to me, it seems that the changes made to McpHub.ts exposed some issues with jest, but the changes were completely unrelated.

I solved the problems with the test to prevent this PR from causing issues when merged.

I also moved the translations and made the MCP buttons responsive since they were overflowing.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 6, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 6, 2025
@taylorwilsdon
Copy link
Contributor Author

taylorwilsdon commented Jun 6, 2025

Thank you @taylorwilsdon!

Looks good to me, it seems that the changes made to McpHub.ts exposed some issues with jest, but the changes were completely unrelated.

I solved the problems with the test to prevent this PR from causing issues when merged.

I also moved the translations and made the MCP buttons responsive since they were overflowing.

Awesome, appreciate you! I think this will be a popular one

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 7, 2025
@daniel-lxs
Copy link
Member

Made some additional changes to solve #4431

@taylorwilsdon taylorwilsdon requested a review from mrubens June 7, 2025 23:16
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

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

Thank you!

@mrubens mrubens merged commit 3f54efc into RooCodeInc:main Jun 8, 2025
8 of 9 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 8, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request lgtm This PR has been approved by a maintainer PR - Needs Review size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

4 participants