⚠️ 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.

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

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

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

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.

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

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.

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.

@aheizi

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

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