Skip to content

Conversation

@aheizi
Copy link
Contributor

@aheizi aheizi commented Apr 16, 2025

Context

To support Streamable HTTP transport more easily later

Restructured the MCP (Model Context Protocol) server implementation to improve code organization and maintainability. The goal is to make the codebase more modular and easier to extend while preserving all existing functionality.

Previous challenges in the codebase:

  • All functionality was concentrated in a single large file (1200+ lines)
  • Mixed concerns made it difficult to maintain and debug
  • Connection handling logic was tightly coupled
  • Duplicate code across different connection types
  • Complex error handling scattered throughout the code
  • Configuration validation mixed with business logic

Implementation

Reorganized the codebase into a clearer directory structure:

├── connection/                 # Connection management related modules
│   ├── handlers/              # Connection type specific handlers
│   │   ├── SseHandler.ts     # Server-Sent Events handler (221 lines)
│   │   └── StdioHandler.ts   # Standard I/O handler (253 lines)
│   ├── ConnectionManager.ts   # Manages connection lifecycle (194 lines)
│   ├── ConnectionFactory.ts   # Creates and configures connections (251 lines)
│   ├── ConnectionHandler.ts   # Connection handler interface (36 lines)
│   ├── FileWatcher.ts        # File monitoring for auto-restart (75 lines)
│   └── index.ts              # Module exports (11 lines)
├── config/                    # Configuration management
│   ├── ConfigManager.ts      # Manages configurations (368 lines)
│   ├── validation.ts         # Configuration validation (70 lines)
│   ├── types.ts             # Configuration type definitions (17 lines)
│   └── index.ts             # Module exports (3 lines)
├── McpHub.ts                 # Core hub implementation (374 lines)
├── McpServerManager.ts       # Server management and coordination (84 lines)
└── types.ts                  # Shared type definitions (122 lines)

Key improvements:

  • Clear separation of concerns with dedicated modules
  • Better organized connection handling with specific handlers
  • Centralized configuration management
  • Improved maintainability through modular design
  • Unified error handling strategy
  • Reduced code duplication

The restructuring maintains all existing functionality while providing a more organized and extensible foundation.

Screenshots

config settings verification

config_compressed.mov

Core function verification

core-function_compressed.mov

How to Test

Manual testing following aspects:

  • Configuration validation
    • Basic server settings:
      • Toggle alwaysAllow tools and verify changes take effect
      • Modify timeout settings and verify connection behavior
      • Change disabled status and verify server state
      • Update watchPaths and verify file monitoring
    • Connection-specific settings:
      • For SSE servers:
        • Modify url and verify connection
        • Update headers and verify request headers
      • For StdIO servers:
        • Change command and verify process launch
        • Modify args and verify command arguments
        • Update env variables and verify environment
        • Change cwd and verify working directory
  • Project and global configuration testing
    • Verify project-level configuration overrides
    • Test global configuration defaults
  • Cross-platform verification
  • MCP functionality verification
    • Tool execution
    • Resource management

Get in Touch

aheizi


Important

Refactor MCP server implementation for improved modularity, introducing new connection and configuration management classes while maintaining existing functionality.

  • Behavior:
    • Refactors MCP server implementation for better modularity and maintainability.
    • Introduces new directory structure with connection, config, and types modules.
    • Maintains existing functionality while improving organization.
  • Connection Management:
    • Adds ConnectionFactory, ConnectionManager, SseHandler, and StdioHandler for handling different connection types.
    • Implements FileWatcher for monitoring file changes.
  • Configuration Management:
    • Introduces ConfigManager for handling global and project-level configurations.
    • Adds ConfigChangeEvent and ConfigChangeListener for configuration change notifications.
  • Error Handling:
    • Centralizes error handling in McpHub and connection handlers.
  • Testing:
    • Updates tests in McpHub.test.ts to reflect new structure and functionality.

This description was created by Ellipsis for 2a08412. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Apr 16, 2025

⚠️ No Changeset found

Latest commit: 40ff84c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@aheizi aheizi closed this Apr 16, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 16, 2025
@aheizi aheizi reopened this Apr 16, 2025
@github-project-automation github-project-automation bot moved this from Done to New in Roo Code Roadmap Apr 16, 2025
@aheizi aheizi marked this pull request as ready for review April 16, 2025 13:45
@aheizi aheizi requested review from cte and mrubens as code owners April 16, 2025 13:45
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Apr 16, 2025
@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Apr 16, 2025

Hello @aheizi,

Thank you for your work on restructuring the MCP server implementation. This pull request involves significant changes across multiple components, including connection management, configuration management, and handlers. While these changes aim to improve modularity, it might be beneficial to split the pull request into smaller, more focused ones to facilitate easier review and integration.

Here are some suggestions on how the changes could be split:

  1. Configuration Management: Changes related to the introduction and implementation of the ConfigManager and associated configuration handling.
  2. Connection Management: Updates involving the ConnectionManager, ConnectionFactory, and related connection handlers (SseHandler, StdioHandler).
  3. Testing Updates: Modifications to test files to accommodate the new structure and mock configurations.

Please consider these suggestions and let us know if you have any questions or need further assistance.

Best regards,
Ellipsis

@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap Apr 17, 2025
aheizi added 7 commits April 18, 2025 22:57
# Conflicts:
#	src/core/Cline.ts
# Conflicts:
#	locales/ca/README.md
#	locales/de/README.md
#	locales/es/README.md
#	locales/fr/README.md
#	locales/hi/README.md
#	locales/it/README.md
#	locales/ja/README.md
#	locales/ko/README.md
#	locales/pl/README.md
#	locales/pt-BR/README.md
#	locales/tr/README.md
#	locales/vi/README.md
#	locales/zh-CN/README.md
#	locales/zh-TW/README.md
#	src/core/Cline.ts
#	src/core/__tests__/Cline.test.ts
# Conflicts:
#	src/services/mcp/McpHub.ts
#	src/services/mcp/__tests__/McpHub.test.ts
@mrubens mrubens self-assigned this May 5, 2025
@aheizi
Copy link
Contributor Author

aheizi commented May 9, 2025

I found there's a bit of a problem with the tool's parameters, let me fix it.

@hannesrudolph hannesrudolph moved this from New to PR [Greenlit] in Roo Code Roadmap May 20, 2025
@hannesrudolph hannesrudolph moved this from TEMP to Needs Preliminary Review in Roo Code Roadmap May 27, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented May 29, 2025

Hey @aheizi, Sorry again for taking so long to get back to you, I really like the idea to split the currently monolithic MCP Hub implementation.

However since this refactor is so big it might be a good idea to add a feature flag to this to hopefully catch any bugs or breaking changes, something like:

// In McpHub.ts
export class McpHub {
    constructor(provider: ClineProvider, useNewArchitecture = false) {
        if (useNewArchitecture) {
            // Initialize with new modular approach
        } else {
            // Keep existing implementation
        }
    }
}

I would like to know your opinion. Hopefully we can get your PR reviewed this way.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap May 29, 2025
@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Draft / In Progress] in Roo Code Roadmap May 29, 2025
@JabolDev
Copy link

JabolDev commented Jun 1, 2025

Hey @aheizi, Sorry again for taking so long to get back to you, I really like the idea to split the currently monolithic MCP Hub implementation.

However since this refactor is so big it might be a good idea to add a feature flag to this to hopefully catch any bugs or breaking changes, something like:

// In McpHub.ts
export class McpHub {
    constructor(provider: ClineProvider, useNewArchitecture = false) {
        if (useNewArchitecture) {
            // Initialize with new modular approach
        } else {
            // Keep existing implementation
        }
    }
}

I would like to know your opinion. Hopefully we can get your PR reviewed this way.

This is very stupid idea bro , are you drunk

@daniel-lxs
Copy link
Member

daniel-lxs commented Jun 1, 2025

Hey @JabolDev ,

I don't think there's a need to be rude.

Feature flags are widely used to test new features and assist with the integration of large implementations/refactorings.

Take a look: https://martinfowler.com/articles/feature-toggles.html

I think this is a good way to integrate this PR but it's not the only solution, that's why I asked the author what they thought about it.

Let me know if you have any questions.

@aheizi
Copy link
Contributor Author

aheizi commented Jun 1, 2025

Hey @aheizi, Sorry again for taking so long to get back to you, I really like the idea to split the currently monolithic MCP Hub implementation.

However since this refactor is so big it might be a good idea to add a feature flag to this to hopefully catch any bugs or breaking changes, something like:

// In McpHub.ts
export class McpHub {
    constructor(provider: ClineProvider, useNewArchitecture = false) {
        if (useNewArchitecture) {
            // Initialize with new modular approach
        } else {
            // Keep existing implementation
        }
    }
}

I would like to know your opinion. Hopefully we can get your PR reviewed this way.

Hi, @daniel-lxs Thank you for taking the time to review this PR!

I think this idea is good and can avoid some risks. I'm very willing to have a try like this.

What kind of grayscale strategy should we adopt? I haven't figured this part out yet.

@daniel-lxs
Copy link
Member

@aheizi

Hey, I think using something like this #4226 would be awesome.

@aheizi aheizi closed this Jun 3, 2025
@github-project-automation github-project-automation bot moved this from PR [Draft / In Progress] to Done in Roo Code Roadmap Jun 3, 2025
@github-project-automation github-project-automation bot moved this from PR [Greenlit] to Done in Roo Code Roadmap Jun 3, 2025
@aheizi
Copy link
Contributor Author

aheizi commented Jun 3, 2025

Seeing that http streaming is already supported, I'm already tired and don't want to tinker anymore.
If there is still time in the future, I will continue to move forward. In this Ai era where everyone is very impetuous, refactoring seems meaningless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request PR - Draft / In Progress size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants