Add configurable MCP server IP address and port instead of hard coded http://127.0.0.1:50300/sse#25
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configurable MCP server IP address and port settings to replace the hardcoded http://+:50300/sse URL. The implementation introduces a configuration dialog, persistent JSON-based settings storage, and helper methods for managing the configuration.
Key Changes:
- New configuration infrastructure with JSON persistence to
mcp_config.json - Windows Forms dialog for user-friendly configuration management with real-time URL preview
- Integration of configuration loading into server startup and menu system
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
McpServerConfig.cs |
New configuration class handling IP/port settings, JSON persistence, and validation logic with default values (+:50300) |
McpConfigDialog.cs |
New Windows Forms dialog providing UI for IP/port configuration with live URL preview and input validation |
Plugin.Menus.cs |
Added "Configure MCP Server..." menu item with STA thread handling for dialog display |
Plugin.Commands.cs |
Updated server initialization to load configuration; added helper methods for config management; includes unrelated GetCallStack refactoring |
MCPServer.cs |
Modified constructors to accept McpServerConfig parameter and use configurable URLs for HTTP listener |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var t = new Thread(() => | ||
| { | ||
| using (var dialog = new McpConfigDialog(config)) | ||
| { | ||
| if (dialog.ShowDialog() == DialogResult.OK) | ||
| { | ||
| // Update and save configuration | ||
| config.IpAddress = dialog.IpAddress; | ||
| config.Port = dialog.Port; | ||
| Plugin.SetMcpServerConfig(config); | ||
|
|
||
| MessageBox.Show( | ||
| $"Configuration saved.\n\nNew URL: {config.GetDisplayUrl()}\n\nPlease restart the MCP server for changes to take effect.", | ||
| "MCP Server Configuration", | ||
| MessageBoxButtons.OK, | ||
| MessageBoxIcon.Information); | ||
| } | ||
| } | ||
| }); | ||
| t.SetApartmentState(ApartmentState.STA); | ||
| t.Start(); | ||
| t.Join(); |
There was a problem hiding this comment.
The thread is being joined synchronously (t.Join()), which blocks the calling thread until the dialog is closed. This defeats the purpose of creating a separate thread. Consider using Application.Run() directly on the current thread if it's already STA, or check the apartment state before creating a new thread. If you must use a new thread, consider whether blocking the caller is the intended behavior.
| /// </summary> | ||
| public string GetDisplayUrl() | ||
| { | ||
| string displayIp = IpAddress == "+" ? "127.0.0.1" : IpAddress; |
There was a problem hiding this comment.
The IP address validation accepts "" as valid, but the GetDisplayUrl() method only handles "+" for display conversion. When "" is used, it will be displayed as-is in the URL ("http://:port/sse"), which is not a valid connectable URL. The display conversion logic should also handle "" the same way as "+".
| string displayIp = IpAddress == "+" ? "127.0.0.1" : IpAddress; | |
| string displayIp = (IpAddress == "+" || IpAddress == "*") ? "127.0.0.1" : IpAddress; |
| private void UpdateUrlPreview() | ||
| { | ||
| string ip = string.IsNullOrWhiteSpace(txtIpAddress.Text) ? "+" : txtIpAddress.Text.Trim(); | ||
| string displayIp = (ip == "+" || ip == "*") ? "127.0.0.1" : ip; | ||
| lblCurrentUrl.Text = $"http://{displayIp}:{(int)numPort.Value}/sse"; |
There was a problem hiding this comment.
The URL preview conversion logic only handles "+" but should also handle "", which is also accepted as a valid wildcard address. When "" is entered, it should be displayed as "127.0.0.1" just like "+".
| public static void SetMcpServerConfig(McpServerConfig config) | ||
| { | ||
| GMcpServerConfig = config; | ||
| config.Save(); |
There was a problem hiding this comment.
The static GMcpServerConfig field is accessed and modified without synchronization from both the menu thread (via ConfigureMCPServer) and command thread (via cbStartMCPServer). This creates a potential race condition. Consider adding lock protection or using a thread-safe pattern for accessing this shared state.
| public void Save() | ||
| { | ||
| try | ||
| { | ||
| var serializer = new JavaScriptSerializer(); | ||
| var json = serializer.Serialize(this); | ||
|
|
||
| // Ensure directory exists | ||
| var dir = Path.GetDirectoryName(ConfigPath); | ||
| if (!string.IsNullOrEmpty(dir) && !Directory.Exists(dir)) | ||
| { | ||
| Directory.CreateDirectory(dir); | ||
| } | ||
|
|
||
| File.WriteAllText(ConfigPath, json); | ||
| Console.WriteLine($"[McpServerConfig] Configuration saved to: {ConfigPath}"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.WriteLine($"[McpServerConfig] Error saving config: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
[nitpick] The configuration file write operation (File.WriteAllText) could fail due to file system permissions, disk space, or file locking issues. However, exceptions are caught and logged, which is a good practice. Consider whether the user should be notified with a visible error message (MessageBox) in addition to console logging, since configuration save failures during the interactive dialog flow may otherwise go unnoticed by the user.
| IntPtr ptrModule = IntPtr.Zero; | ||
| IntPtr ptrLabel = IntPtr.Zero; | ||
| IntPtr ptrComment = IntPtr.Zero; | ||
| BRIDGE_ADDRINFO_NATIVE addrInfo = new BRIDGE_ADDRINFO_NATIVE(); // Must be NATIVE struct | ||
|
|
||
| for (int i = 0; i < callstackFrames.Count; i++) | ||
| { |
There was a problem hiding this comment.
Moving the memory allocation outside the loop creates a resource leak. The IntPtr variables are allocated once but freed in every iteration, and the final iteration's allocated memory will never be freed. The original code (allocating inside the loop) ensures all allocated memory is freed in the corresponding finally block.
| IntPtr ptrModule = IntPtr.Zero; | |
| IntPtr ptrLabel = IntPtr.Zero; | |
| IntPtr ptrComment = IntPtr.Zero; | |
| BRIDGE_ADDRINFO_NATIVE addrInfo = new BRIDGE_ADDRINFO_NATIVE(); // Must be NATIVE struct | |
| for (int i = 0; i < callstackFrames.Count; i++) | |
| { | |
| BRIDGE_ADDRINFO_NATIVE addrInfo = new BRIDGE_ADDRINFO_NATIVE(); // Must be NATIVE struct | |
| for (int i = 0; i < callstackFrames.Count; i++) | |
| { | |
| IntPtr ptrModule = IntPtr.Zero; | |
| IntPtr ptrLabel = IntPtr.Zero; | |
| IntPtr ptrComment = IntPtr.Zero; |
| Top = 20, | ||
| Width = 80, | ||
| TextAlign = ContentAlignment.MiddleRight | ||
| }; |
There was a problem hiding this comment.
Disposable 'Label' is created but not disposed.
| }; | |
| }; | |
| Controls.Add(lblIp); |
| var lblIpHelp = new Label | ||
| { | ||
| Text = "(+, *, localhost, or IP)", | ||
| Left = 285, | ||
| Top = 20, | ||
| Width = 110, | ||
| ForeColor = Color.Gray, | ||
| Font = new Font(Font.FontFamily, 8) | ||
| }; |
There was a problem hiding this comment.
Disposable 'Label' is created but not disposed.
| var lblPort = new Label | ||
| { | ||
| Text = "Port:", | ||
| Left = 15, | ||
| Top = 55, | ||
| Width = 80, | ||
| TextAlign = ContentAlignment.MiddleRight | ||
| }; |
There was a problem hiding this comment.
Disposable 'Label' is created but not disposed.
| var lblUrlTitle = new Label | ||
| { | ||
| Text = "Server URL:", | ||
| Left = 15, | ||
| Top = 95, | ||
| Width = 80, | ||
| TextAlign = ContentAlignment.MiddleRight | ||
| }; |
There was a problem hiding this comment.
Disposable 'Label' is created but not disposed.
|
Thank you, I will review the change and merge it if I don't have any follow up questions. |
Description
This PR adds the ability to configure the MCP server's IP address and port through a plugin UI dialog, replacing the previously hardcoded
http://+:50300/sseURL.Changes
New Files
McpServerConfig.cs- Configuration class that stores IP address and port settings, with JSON persistence tomcp_config.jsonin the plugin directory.McpConfigDialog.cs- Windows Forms dialog for configuring settings via the plugin menu.Modified Files
MCPServer.cs- UpdatedSimpleMcpServerconstructor to acceptMcpServerConfigparameter for configurable HTTP listener URL.Plugin.Commands.cs- UpdatedcbStartMCPServerto load and use configuration; addedGetMcpServerConfig()andSetMcpServerConfig()helper methods.Plugin.Menus.cs- Added "Configure MCP Server..." menu item under Plugins menu.Features
mcp_config.jsonand persist across sessions.Supported IP Address Values
+or*urlacl)127.0.0.1orlocalhost192.168.1.100)Usage
Testing
Breaking Changes
None. Default values (
+:50300) match the previous hardcoded behavior.