Feature/onboard window #411
Draft
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MCP Status Checks Window to help users diagnose and verify their MCP integration setup. The window provides 7 diagnostic checks covering client configuration, server connection, version compatibility, enabled tools, and tool execution tracking.
Changes:
- New McpStatusChecksWindow with 7 onboarding status checks (3 live, 4 stub/pending)
- Dynamic UI with expandable cards, mini/full view toggle, and visual status indicators
- Integration with MainWindow showing live status check counter
- Helper methods to retrieve and filter client configurations across platforms
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| McpStatusChecksWindow.cs | Core implementation with 7 status check methods, UI card generation, and reactive updates |
| McpStatusChecksWindow.uxml | UXML layout defining mini/full views and scroll container for status cards |
| McpStatusChecksWindow.uss | Comprehensive styling for cards, icons, colors, and responsive states |
| MainWindow.uxml | Added Status Checks section with counter label and Open button; fixed apostrophe encoding |
| MainWindowEditor.CreateGUI.cs | Status check counter logic with reactive updates from connection state and tool manager |
| MainWindowEditor.ClientConfigure.cs | New public methods GetAllClientConfigs() and GetConfiguredClients() for cross-platform client retrieval |
| *.meta files | Unity metadata files for new assets |
Comment on lines
117
to
145
| public static ClientConfig[] GetAllClientConfigs() | ||
| { | ||
| #if UNITY_EDITOR_WIN | ||
| return GetClientConfigsWindows(); | ||
| #elif UNITY_EDITOR_OSX || UNITY_EDITOR_LINUX | ||
| return GetClientConfigsMacAndLinux(); | ||
| #else | ||
| return GetClientConfigsWindows(); | ||
| #endif | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns a list of configured clients (where IsConfigured() returns true). | ||
| /// </summary> | ||
| public static List<ClientConfig> GetConfiguredClients() | ||
| { | ||
| var allConfigs = GetAllClientConfigs(); | ||
| var configured = new List<ClientConfig>(); | ||
| foreach (var config in allConfigs) | ||
| { | ||
| try | ||
| { | ||
| if (config.IsConfigured()) | ||
| configured.Add(config); | ||
| } | ||
| catch { /* Ignore */ } | ||
| } | ||
| return configured; | ||
| } |
There was a problem hiding this comment.
The new GetAllClientConfigs and GetConfiguredClients methods lack test coverage. Given that the repository has comprehensive test coverage for similar client configuration functionality (see MainWindowEditor.ClientConfigureTests.cs), these new methods should also be tested to ensure they correctly retrieve and filter client configurations across different platforms.
Consider adding tests to verify that GetAllClientConfigs returns the correct configurations for each platform and that GetConfiguredClients properly filters to only configured clients.
Comment on lines
29
to
569
| public class McpStatusChecksWindow : McpWindowBase | ||
| { | ||
| private static readonly string[] _windowUxmlPaths = EditorAssetLoader.GetEditorAssetPaths("Editor/UI/uxml/McpStatusChecksWindow.uxml"); | ||
| private static readonly string[] _windowUssPaths = EditorAssetLoader.GetEditorAssetPaths("Editor/UI/uss/McpStatusChecksWindow.uss"); | ||
|
|
||
| protected override string[] WindowUxmlPaths => _windowUxmlPaths; | ||
| protected override string[] WindowUssPaths => _windowUssPaths; | ||
| protected override string WindowTitle => "MCP Status Checks"; | ||
|
|
||
| private readonly CompositeDisposable _disposables = new(); | ||
| private VisualElement? _statusItemsContainer; | ||
| private VisualElement? _miniViewContainer; | ||
| private VisualElement? _fullViewContainer; | ||
| private bool _isExpanded = true; | ||
|
|
||
| // Track tool execution count (resets on domain reload) | ||
| private static int _toolExecutionCount = 0; | ||
|
|
||
| /// <summary> | ||
| /// Status of a single check item. | ||
| /// </summary> | ||
| public enum CheckStatus | ||
| { | ||
| Success, // Green - check passed | ||
| Error, // Red - check failed, action required | ||
| Pending // Gray - optional or awaiting | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Data model for a status check item. | ||
| /// </summary> | ||
| public class StatusCheckItem | ||
| { | ||
| public string Id { get; set; } = string.Empty; | ||
| public string Title { get; set; } = string.Empty; | ||
| public string Subtitle { get; set; } = string.Empty; | ||
| public string Description { get; set; } = string.Empty; | ||
| public CheckStatus Status { get; set; } = CheckStatus.Pending; | ||
|
|
||
| public StatusCheckItem(string id, string title, string subtitle = "", string description = "", CheckStatus status = CheckStatus.Pending) | ||
| { | ||
| Id = id; | ||
| Title = title; | ||
| Subtitle = subtitle; | ||
| Description = description; | ||
| Status = status; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Shows the MCP Status Checks window. | ||
| /// </summary> | ||
| public static void ShowWindow() | ||
| { | ||
| var window = GetWindow<McpStatusChecksWindow>("MCP Status Checks"); | ||
| window.SetupWindowWithIcon(); | ||
| window.minSize = new UnityEngine.Vector2(400, 500); | ||
| window.Show(); | ||
| window.Focus(); | ||
| } | ||
|
|
||
| protected override void OnEnable() | ||
| { | ||
| base.OnEnable(); | ||
| SubscribeToUpdates(); | ||
| } | ||
|
|
||
| private void OnDisable() | ||
| { | ||
| _disposables.Clear(); | ||
| } | ||
|
|
||
| protected override void OnGUICreated(VisualElement root) | ||
| { | ||
| base.OnGUICreated(root); | ||
|
|
||
| _miniViewContainer = root.Q<VisualElement>("mini-view-container"); | ||
| _fullViewContainer = root.Q<VisualElement>("full-view-container"); | ||
| _statusItemsContainer = root.Q<VisualElement>(className: "status-items-container"); | ||
|
|
||
| if (_statusItemsContainer == null) | ||
| { | ||
| Logger.LogWarning("{method}: status-items-container not found", nameof(OnGUICreated)); | ||
| return; | ||
| } | ||
|
|
||
| // Setup mini view click handler | ||
| if (_miniViewContainer != null) | ||
| { | ||
| _miniViewContainer.RegisterCallback<MouseDownEvent>(evt => ToggleView()); | ||
| } | ||
|
|
||
| // Clear any static cards | ||
| _statusItemsContainer.Clear(); | ||
|
|
||
| // Initial render | ||
| RefreshAllChecks(); | ||
| UpdateViewState(); | ||
| } | ||
|
|
||
| private void ToggleView() | ||
| { | ||
| _isExpanded = !_isExpanded; | ||
| UpdateViewState(); | ||
| } | ||
|
|
||
| private void UpdateViewState() | ||
| { | ||
| if (_miniViewContainer == null || _fullViewContainer == null) | ||
| return; | ||
|
|
||
| _miniViewContainer.style.display = _isExpanded ? DisplayStyle.None : DisplayStyle.Flex; | ||
| _fullViewContainer.style.display = _isExpanded ? DisplayStyle.Flex : DisplayStyle.None; | ||
| } | ||
|
|
||
| private void SubscribeToUpdates() | ||
| { | ||
| _disposables.Clear(); | ||
|
|
||
| // Subscribe to connection state changes | ||
| UnityMcpPlugin.ConnectionState | ||
| .ThrottleLast(TimeSpan.FromMilliseconds(100)) | ||
| .ObserveOnCurrentSynchronizationContext() | ||
| .Subscribe(_ => RefreshAllChecks()) | ||
| .AddTo(_disposables); | ||
|
|
||
| // Subscribe to tool manager updates | ||
| McpPlugin.McpPlugin.DoAlways(plugin => | ||
| { | ||
| var toolManager = plugin.McpManager.ToolManager; | ||
| if (toolManager != null) | ||
| { | ||
| toolManager.OnToolsUpdated | ||
| .ObserveOnCurrentSynchronizationContext() | ||
| .Subscribe(_ => RefreshAllChecks()) | ||
| .AddTo(_disposables); | ||
| } | ||
| }).AddTo(_disposables); | ||
| } | ||
|
|
||
| private void RefreshAllChecks() | ||
| { | ||
| if (_statusItemsContainer == null) | ||
| return; | ||
|
|
||
| var checks = GatherStatusChecks(); | ||
|
|
||
| // Update mini view | ||
| UpdateMiniView(checks); | ||
|
|
||
| // Update stats label in full view | ||
| var statsLabel = _fullViewContainer?.Q<Label>("stats-label"); | ||
| if (statsLabel != null) | ||
| { | ||
| var passedCount = checks.Count(c => c.Status == CheckStatus.Success); | ||
| statsLabel.text = $"({passedCount}/{checks.Count})"; | ||
| } | ||
|
|
||
| // Update full view | ||
| _statusItemsContainer.Clear(); | ||
| foreach (var check in checks) | ||
| { | ||
| var card = CreateStatusCard(check); | ||
| _statusItemsContainer.Add(card); | ||
| } | ||
| } | ||
|
|
||
| private void UpdateMiniView(List<StatusCheckItem> checks) | ||
| { | ||
| var miniDotsContainer = _miniViewContainer?.Q<VisualElement>("mini-dots"); | ||
| var miniLabel = _miniViewContainer?.Q<Label>("mini-label"); | ||
|
|
||
| if (miniDotsContainer == null || miniLabel == null) | ||
| return; | ||
|
|
||
| var passedCount = checks.Count(c => c.Status == CheckStatus.Success); | ||
| miniLabel.text = $"Status ({passedCount}/{checks.Count})"; | ||
|
|
||
| miniDotsContainer.Clear(); | ||
| foreach (var check in checks) | ||
| { | ||
| var dot = new VisualElement(); | ||
| dot.AddToClassList("mini-dot"); | ||
| dot.AddToClassList(check.Status switch | ||
| { | ||
| CheckStatus.Success => "dot-success", | ||
| CheckStatus.Error => "dot-error", | ||
| _ => "dot-pending" | ||
| }); | ||
| miniDotsContainer.Add(dot); | ||
| } | ||
| } | ||
|
|
||
| private List<StatusCheckItem> GatherStatusChecks() | ||
| { | ||
| var checks = new List<StatusCheckItem> | ||
| { | ||
| GetMcpClientConfiguredCheck(), | ||
| GetUnityConnectedCheck(), | ||
| GetVersionHandshakeCheck(), | ||
| GetServerToClientCheck(), | ||
| GetClientLocationCheck(), | ||
| GetEnabledToolsCheck(), | ||
| GetToolExecutedCheck() | ||
| }; | ||
| return checks; | ||
| } | ||
|
|
||
| #region Status Check Methods | ||
|
|
||
| private StatusCheckItem GetMcpClientConfiguredCheck() | ||
| { | ||
| var configuredClients = GetConfiguredClientNames(); | ||
| var count = configuredClients.Count; | ||
| var isConfigured = count > 0; | ||
|
|
||
| var clientList = isConfigured ? string.Join(", ", configuredClients) : ""; | ||
|
|
||
| return new StatusCheckItem( | ||
| id: "mcp-client-configured", | ||
| title: "MCP Client configured", | ||
| subtitle: isConfigured ? $"{count} configured: {clientList}" : "No MCP clients configured", | ||
| description: isConfigured | ||
| ? $"Configured clients: {clientList}. You can configure more clients in the main window using the Configure button." | ||
| : "No MCP clients are configured. Open the main MCP window and click the 'Configure' button next to your preferred MCP client (Claude Desktop, Cursor, etc.) to set up the connection.", | ||
| status: isConfigured ? CheckStatus.Success : CheckStatus.Error | ||
| ); | ||
| } | ||
|
|
||
| private List<string> GetConfiguredClientNames() | ||
| { | ||
| var configuredClients = MainWindowEditor.GetConfiguredClients(); | ||
| var names = new List<string>(); | ||
| foreach (var client in configuredClients) | ||
| { | ||
| names.Add(client.Name); | ||
| } | ||
| return names; | ||
| } | ||
|
|
||
| private StatusCheckItem GetUnityConnectedCheck() | ||
| { | ||
| var connectionState = UnityMcpPlugin.ConnectionState.CurrentValue; | ||
| var isConnected = connectionState == HubConnectionState.Connected; | ||
| var keepConnected = UnityMcpPlugin.KeepConnected; | ||
| var version = UnityMcpPlugin.Version; | ||
|
|
||
| if (isConnected) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "unity-connected", | ||
| title: "Unity connected to MCP Server", | ||
| subtitle: $"Connected (Plugin v{version})", | ||
| description: $"Unity is successfully connected to the MCP Server. Plugin version: {version}", | ||
| status: CheckStatus.Success | ||
| ); | ||
| } | ||
| else if (keepConnected && (connectionState == HubConnectionState.Connecting || connectionState == HubConnectionState.Reconnecting)) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "unity-connected", | ||
| title: "Unity connected to MCP Server", | ||
| subtitle: connectionState == HubConnectionState.Connecting ? "Connecting..." : "Reconnecting...", | ||
| description: "Unity is attempting to connect to the MCP Server. Please wait...", | ||
| status: CheckStatus.Pending | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "unity-connected", | ||
| title: "Unity connected to MCP Server", | ||
| subtitle: "Not connected", | ||
| description: "Unity is not connected to the MCP Server.\n\n" + | ||
| "To fix this:\n" + | ||
| "1. Open the main MCP window (Window → MCP → Main)\n" + | ||
| "2. Click the 'Connect' button\n" + | ||
| "3. Make sure your configured MCP client is running and has this Unity project folder open", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private StatusCheckItem GetVersionHandshakeCheck() | ||
| { | ||
| var version = UnityMcpPlugin.Version; | ||
| var isConnected = UnityMcpPlugin.IsConnected.CurrentValue; | ||
|
|
||
| // Stub: Server version would come from RemoteMcpManagerHub | ||
| var serverVersion = isConnected ? version : "Unknown"; | ||
|
|
||
| if (isConnected) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "version-handshake", | ||
| title: "Version handshake status", | ||
| subtitle: $"Plugin v{version}, Server v{serverVersion}", | ||
| description: $"Plugin and Server versions are compatible.\n\nPlugin version: {version}\nServer version: {serverVersion}", | ||
| status: CheckStatus.Success | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "version-handshake", | ||
| title: "Version handshake status", | ||
| subtitle: "Connect to verify version", | ||
| description: "Cannot verify version compatibility until connected to the MCP Server.\n\n" + | ||
| "If you have issues after connecting, make sure your MCP Server (mcp-tool-unity) is up to date. " + | ||
| "You may need to update it if the plugin was recently updated.", | ||
| status: CheckStatus.Pending | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private StatusCheckItem GetServerToClientCheck() | ||
| { | ||
| var isConnected = UnityMcpPlugin.IsConnected.CurrentValue; | ||
|
|
||
| // Stub: Client name would come from MCP Server tracking | ||
| var clientName = "Unknown"; | ||
|
|
||
| if (isConnected) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "server-to-client", | ||
| title: "MCP Server connected to MCP Client", | ||
| subtitle: $"Awaiting client: {clientName}", | ||
| description: "The MCP Server is running and waiting for an MCP Client connection.\n\n" + | ||
| "Make sure your MCP client (Claude Desktop, Cursor, etc.) is:\n" + | ||
| "1. Running\n" + | ||
| "2. Configured with the correct Unity-MCP server settings\n" + | ||
| "3. Has this Unity project folder open or selected", | ||
| status: CheckStatus.Pending | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "server-to-client", | ||
| title: "MCP Server connected to MCP Client", | ||
| subtitle: "Server not running", | ||
| description: "The MCP Server is not running, so no MCP Client can connect.\n\n" + | ||
| "First, click 'Connect' in the main MCP window to start the server.", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private StatusCheckItem GetClientLocationCheck() | ||
| { | ||
| var unityProjectPath = Environment.CurrentDirectory; | ||
| var isConnected = UnityMcpPlugin.IsConnected.CurrentValue; | ||
| var configuredClients = GetConfiguredClientNames(); | ||
|
|
||
| if (configuredClients.Count > 0) | ||
| { | ||
| var clientList = string.Join(", ", configuredClients); | ||
| return new StatusCheckItem( | ||
| id: "client-location", | ||
| title: "MCP Client location match", | ||
| subtitle: $"Unity: {unityProjectPath}", | ||
| description: $"Unity project path: {unityProjectPath}\n\n" + | ||
| $"Configured clients: {clientList}\n\n" + | ||
| "Make sure your MCP client is running in the same directory. " + | ||
| "Each Unity project generates a unique port based on its folder path, " + | ||
| "so the client must be launched from the correct location.", | ||
| status: isConnected ? CheckStatus.Success : CheckStatus.Pending | ||
| ); | ||
| } | ||
|
|
||
| return new StatusCheckItem( | ||
| id: "client-location", | ||
| title: "MCP Client location match", | ||
| subtitle: "No client config found", | ||
| description: "Could not find any MCP client configuration.\n\n" + | ||
| $"Unity project path: {unityProjectPath}\n\n" + | ||
| "Configure an MCP client first using the 'Configure' button in the main MCP window.", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } | ||
|
|
||
| private StatusCheckItem GetEnabledToolsCheck() | ||
| { | ||
| var mcpPlugin = UnityMcpPlugin.Instance.McpPluginInstance; | ||
| var toolManager = mcpPlugin?.McpManager.ToolManager; | ||
|
|
||
| if (toolManager == null) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "enabled-tools", | ||
| title: "MCP Plugin has at least 1 enabled tool", | ||
| subtitle: "Plugin not initialized", | ||
| description: "The MCP Plugin is not initialized. Try connecting to the MCP Server first.", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } | ||
|
|
||
| var allTools = toolManager.GetAllTools().ToList(); | ||
| var enabledCount = allTools.Count(t => toolManager.IsToolEnabled(t.Name)); | ||
| var totalCount = allTools.Count; | ||
|
|
||
| if (enabledCount > 0) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "enabled-tools", | ||
| title: "MCP Plugin has at least 1 enabled tool", | ||
| subtitle: $"{enabledCount} / {totalCount} tools enabled", | ||
| description: $"You have {enabledCount} out of {totalCount} tools enabled.\n\n" + | ||
| "You can manage enabled tools in Window → MCP → Tools. " + | ||
| "Disabling unused tools saves LLM context tokens.", | ||
| status: CheckStatus.Success | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "enabled-tools", | ||
| title: "MCP Plugin has at least 1 enabled tool", | ||
| subtitle: "0 tools enabled", | ||
| description: "No tools are currently enabled. The MCP client won't be able to perform any actions.\n\n" + | ||
| "To fix this:\n" + | ||
| "1. Go to Window → MCP → Tools\n" + | ||
| "2. Enable at least one tool by toggling it on", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private StatusCheckItem GetToolExecutedCheck() | ||
| { | ||
| // This is always gray/optional - just an indicator | ||
| return new StatusCheckItem( | ||
| id: "tool-executed", | ||
| title: "MCP Tool executed", | ||
| subtitle: _toolExecutionCount > 0 ? $"{_toolExecutionCount} executed this session" : "No tools executed yet", | ||
| description: _toolExecutionCount > 0 | ||
| ? $"MCP tools have been executed {_toolExecutionCount} times this session. This confirms that the connection is working correctly." | ||
| : "No MCP tools have been executed yet this session. This counter increments each time an MCP client successfully calls a tool.", | ||
| status: CheckStatus.Pending // Always pending/gray - optional indicator | ||
| ); | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| /// <summary> | ||
| /// Call this method to increment the tool execution counter. | ||
| /// Should be called after a tool is successfully executed. | ||
| /// </summary> | ||
| public static void NotifyToolExecuted() | ||
| { | ||
| _toolExecutionCount++; | ||
| } | ||
|
|
||
| private VisualElement CreateStatusCard(StatusCheckItem item) | ||
| { | ||
| var card = new VisualElement(); | ||
| card.AddToClassList("status-card"); | ||
| card.name = item.Id; | ||
|
|
||
| // Status-based styling | ||
| card.AddToClassList(item.Status switch | ||
| { | ||
| CheckStatus.Success => "success", | ||
| CheckStatus.Error => "error", | ||
| _ => "pending" | ||
| }); | ||
|
|
||
| if (item.Status == CheckStatus.Error) | ||
| { | ||
| var errorAccent = new VisualElement(); | ||
| errorAccent.AddToClassList("error-accent"); | ||
| card.Add(errorAccent); | ||
| } | ||
|
|
||
| // Icon container | ||
| var iconContainer = new VisualElement(); | ||
| iconContainer.AddToClassList("status-icon-container"); | ||
| iconContainer.AddToClassList(item.Status switch | ||
| { | ||
| CheckStatus.Success => "success-icon", | ||
| CheckStatus.Error => "error-icon", | ||
| _ => "pending-icon" | ||
| }); | ||
|
|
||
| var iconText = new Label | ||
| { | ||
| text = item.Status switch | ||
| { | ||
| CheckStatus.Success => "✔", | ||
| CheckStatus.Error => "✖", | ||
| _ => "◯" | ||
| } | ||
| }; | ||
| iconText.AddToClassList("status-icon-text"); | ||
| iconContainer.Add(iconText); | ||
| card.Add(iconContainer); | ||
|
|
||
| // Content container | ||
| var contentContainer = new VisualElement(); | ||
| contentContainer.AddToClassList("status-content"); | ||
|
|
||
| var titleLabel = new Label(item.Title); | ||
| titleLabel.AddToClassList("status-title"); | ||
| contentContainer.Add(titleLabel); | ||
|
|
||
| var subtitleLabel = new Label(item.Subtitle); | ||
| subtitleLabel.AddToClassList("status-subtitle"); | ||
| if (item.Status == CheckStatus.Error) | ||
| subtitleLabel.AddToClassList("error-text"); | ||
| contentContainer.Add(subtitleLabel); | ||
|
|
||
| // Description (expandable, shown for errors) | ||
| if (!string.IsNullOrEmpty(item.Description)) | ||
| { | ||
| var descriptionLabel = new Label(item.Description); | ||
| descriptionLabel.AddToClassList("status-description"); | ||
| if (item.Status == CheckStatus.Error) | ||
| { | ||
| descriptionLabel.style.display = DisplayStyle.Flex; | ||
| } | ||
| else | ||
| { | ||
| descriptionLabel.style.display = DisplayStyle.None; | ||
| } | ||
| contentContainer.Add(descriptionLabel); | ||
|
|
||
| // Click to toggle description | ||
| card.RegisterCallback<MouseDownEvent>(evt => | ||
| { | ||
| var isVisible = descriptionLabel.style.display == DisplayStyle.Flex; | ||
| descriptionLabel.style.display = isVisible ? DisplayStyle.None : DisplayStyle.Flex; | ||
| }); | ||
| } | ||
|
|
||
| card.Add(contentContainer); | ||
|
|
||
| return card; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The McpStatusChecksWindow class and its complex status check logic lack test coverage. The repository has comprehensive test coverage for other UI windows and editor functionality (see Tests/Editor/UI directory), but this new window with its 7 different status checks and dynamic UI generation is not tested.
Consider adding tests to verify the status check logic, especially for edge cases like disconnected states, missing tool managers, and the various client configuration scenarios. Tests should cover both the check logic methods (GetMcpClientConfiguredCheck, GetUnityConnectedCheck, etc.) and the UI card generation.
| if (config.IsConfigured()) | ||
| configured.Add(config); | ||
| } | ||
| catch { /* Ignore */ } |
There was a problem hiding this comment.
The empty catch block silently swallows all exceptions when checking if a client is configured. This makes debugging difficult if IsConfigured() throws an unexpected exception. While it may be intentional to ignore configuration errors, completely silent failures can hide bugs or misconfigurations.
Consider logging the exception at a debug or warning level, or at minimum add a comment explaining why exceptions are intentionally ignored here.
Suggested change
| catch { /* Ignore */ } | |
| catch (Exception ex) | |
| { | |
| Debug.LogWarning($"[Unity MCP] Failed to evaluate IsConfigured() for a client: {ex}"); | |
| } |
Comment on lines
154
to
231
| private static ClientConfig[] GetClientConfigsWindows() | ||
| { | ||
| var projectRootPath = GetProjectRootPath(); | ||
| return new ClientConfig[] | ||
| { | ||
| new JsonClientConfig( | ||
| name: "Claude Code", | ||
| configPath: Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), | ||
| ".claude.json" | ||
| ), | ||
| bodyPath: $"projects{Consts.MCP.Server.BodyPathDelimiter}" | ||
| + $"{projectRootPath}{Consts.MCP.Server.BodyPathDelimiter}" | ||
| + Consts.MCP.Server.DefaultBodyPath | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Claude Desktop", | ||
| configPath: Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), | ||
| "Claude", | ||
| "claude_desktop_config.json" | ||
| ), | ||
| bodyPath: Consts.MCP.Server.DefaultBodyPath | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Visual Studio Code (Copilot)", | ||
| configPath: Path.Combine( | ||
| ".vscode", | ||
| "mcp.json" | ||
| ), | ||
| bodyPath: "servers" | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Visual Studio (Copilot)", | ||
| configPath: Path.Combine( | ||
| ".vs", | ||
| "mcp.json" | ||
| ), | ||
| bodyPath: "servers" | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Cursor", | ||
| configPath: Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), | ||
| ".cursor", | ||
| "mcp.json" | ||
| ), | ||
| bodyPath: Consts.MCP.Server.DefaultBodyPath | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Gemini", | ||
| configPath: Path.Combine( | ||
| ".gemini", | ||
| "settings.json" | ||
| ), | ||
| bodyPath: Consts.MCP.Server.DefaultBodyPath | ||
| ), | ||
| new JsonClientConfig( | ||
| name: "Antigravity (Gemini)", | ||
| configPath: Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), | ||
| ".gemini", | ||
| "antigravity", | ||
| "mcp_config.json" | ||
| ), | ||
| bodyPath: Consts.MCP.Server.DefaultBodyPath | ||
| ), | ||
| new TomlClientConfig( | ||
| name: "Codex", | ||
| configPath: Path.Combine( | ||
| Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), | ||
| ".codex", | ||
| "config.toml" | ||
| ), | ||
| bodyPath: "mcp_servers" | ||
| ) | ||
| }; | ||
| } |
There was a problem hiding this comment.
The GetClientConfigsWindows method duplicates the entire client configuration array that already exists in the ConfigureClientsWindows method (lines 36-109). Similarly, GetClientConfigsMacAndLinux duplicates ConfigureClientsMacAndLinux (lines 316-391). This creates a maintenance burden where any changes to client configurations must be made in multiple places.
Consider refactoring to eliminate this duplication. The ConfigureClientsWindows and ConfigureClientsMacAndLinux methods should call GetClientConfigsWindows and GetClientConfigsMacAndLinux respectively, instead of defining the arrays inline.
Comment on lines
147
to
152
| private static string GetProjectRootPath() | ||
| { | ||
| return Application.dataPath.EndsWith("/Assets") | ||
| ? Application.dataPath.Substring(0, Application.dataPath.Length - "/Assets".Length) | ||
| : Application.dataPath; | ||
| } |
There was a problem hiding this comment.
The GetProjectRootPath method is duplicated - it exists as both a static private method (line 147) and an instance property ProjectRootPath (line 30). The logic is identical in both cases. This creates unnecessary duplication and potential for inconsistency if one is updated without the other.
Consider removing the static method and using the instance property, or making the static method the single source of truth if it needs to be accessed from static contexts.
Comment on lines
+189
to
+191
| foreach (var check in checks) | ||
| { | ||
| var card = CreateStatusCard(check); |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
Suggested change
| foreach (var check in checks) | |
| { | |
| var card = CreateStatusCard(check); | |
| var statusCards = checks.Select(CreateStatusCard); | |
| foreach (var card in statusCards) | |
| { |
Comment on lines
320
to
341
| if (isConnected) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "version-handshake", | ||
| title: "Version handshake status", | ||
| subtitle: $"Plugin v{version}, Server v{serverVersion}", | ||
| description: $"Plugin and Server versions are compatible.\n\nPlugin version: {version}\nServer version: {serverVersion}", | ||
| status: CheckStatus.Success | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "version-handshake", | ||
| title: "Version handshake status", | ||
| subtitle: "Connect to verify version", | ||
| description: "Cannot verify version compatibility until connected to the MCP Server.\n\n" + | ||
| "If you have issues after connecting, make sure your MCP Server (mcp-tool-unity) is up to date. " + | ||
| "You may need to update it if the plugin was recently updated.", | ||
| status: CheckStatus.Pending | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Comment on lines
351
to
375
| if (isConnected) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "server-to-client", | ||
| title: "MCP Server connected to MCP Client", | ||
| subtitle: $"Awaiting client: {clientName}", | ||
| description: "The MCP Server is running and waiting for an MCP Client connection.\n\n" + | ||
| "Make sure your MCP client (Claude Desktop, Cursor, etc.) is:\n" + | ||
| "1. Running\n" + | ||
| "2. Configured with the correct Unity-MCP server settings\n" + | ||
| "3. Has this Unity project folder open or selected", | ||
| status: CheckStatus.Pending | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "server-to-client", | ||
| title: "MCP Server connected to MCP Client", | ||
| subtitle: "Server not running", | ||
| description: "The MCP Server is not running, so no MCP Client can connect.\n\n" + | ||
| "First, click 'Connect' in the main MCP window to start the server.", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Suggested change
| if (isConnected) | |
| { | |
| return new StatusCheckItem( | |
| id: "server-to-client", | |
| title: "MCP Server connected to MCP Client", | |
| subtitle: $"Awaiting client: {clientName}", | |
| description: "The MCP Server is running and waiting for an MCP Client connection.\n\n" + | |
| "Make sure your MCP client (Claude Desktop, Cursor, etc.) is:\n" + | |
| "1. Running\n" + | |
| "2. Configured with the correct Unity-MCP server settings\n" + | |
| "3. Has this Unity project folder open or selected", | |
| status: CheckStatus.Pending | |
| ); | |
| } | |
| else | |
| { | |
| return new StatusCheckItem( | |
| id: "server-to-client", | |
| title: "MCP Server connected to MCP Client", | |
| subtitle: "Server not running", | |
| description: "The MCP Server is not running, so no MCP Client can connect.\n\n" + | |
| "First, click 'Connect' in the main MCP window to start the server.", | |
| status: CheckStatus.Error | |
| ); | |
| } | |
| var subtitle = isConnected | |
| ? $"Awaiting client: {clientName}" | |
| : "Server not running"; | |
| var description = isConnected | |
| ? "The MCP Server is running and waiting for an MCP Client connection.\n\n" + | |
| "Make sure your MCP client (Claude Desktop, Cursor, etc.) is:\n" + | |
| "1. Running\n" + | |
| "2. Configured with the correct Unity-MCP server settings\n" + | |
| "3. Has this Unity project folder open or selected" | |
| : "The MCP Server is not running, so no MCP Client can connect.\n\n" + | |
| "First, click 'Connect' in the main MCP window to start the server."; | |
| var status = isConnected ? CheckStatus.Pending : CheckStatus.Error; | |
| return new StatusCheckItem( | |
| id: "server-to-client", | |
| title: "MCP Server connected to MCP Client", | |
| subtitle: subtitle, | |
| description: description, | |
| status: status | |
| ); |
Comment on lines
+430
to
+455
|
|
||
| if (enabledCount > 0) | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "enabled-tools", | ||
| title: "MCP Plugin has at least 1 enabled tool", | ||
| subtitle: $"{enabledCount} / {totalCount} tools enabled", | ||
| description: $"You have {enabledCount} out of {totalCount} tools enabled.\n\n" + | ||
| "You can manage enabled tools in Window → MCP → Tools. " + | ||
| "Disabling unused tools saves LLM context tokens.", | ||
| status: CheckStatus.Success | ||
| ); | ||
| } | ||
| else | ||
| { | ||
| return new StatusCheckItem( | ||
| id: "enabled-tools", | ||
| title: "MCP Plugin has at least 1 enabled tool", | ||
| subtitle: "0 tools enabled", | ||
| description: "No tools are currently enabled. The MCP client won't be able to perform any actions.\n\n" + | ||
| "To fix this:\n" + | ||
| "1. Go to Window → MCP → Tools\n" + | ||
| "2. Enable at least one tool by toggling it on", | ||
| status: CheckStatus.Error | ||
| ); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
Suggested change
| if (enabledCount > 0) | |
| { | |
| return new StatusCheckItem( | |
| id: "enabled-tools", | |
| title: "MCP Plugin has at least 1 enabled tool", | |
| subtitle: $"{enabledCount} / {totalCount} tools enabled", | |
| description: $"You have {enabledCount} out of {totalCount} tools enabled.\n\n" + | |
| "You can manage enabled tools in Window → MCP → Tools. " + | |
| "Disabling unused tools saves LLM context tokens.", | |
| status: CheckStatus.Success | |
| ); | |
| } | |
| else | |
| { | |
| return new StatusCheckItem( | |
| id: "enabled-tools", | |
| title: "MCP Plugin has at least 1 enabled tool", | |
| subtitle: "0 tools enabled", | |
| description: "No tools are currently enabled. The MCP client won't be able to perform any actions.\n\n" + | |
| "To fix this:\n" + | |
| "1. Go to Window → MCP → Tools\n" + | |
| "2. Enable at least one tool by toggling it on", | |
| status: CheckStatus.Error | |
| ); | |
| } | |
| var hasEnabledTools = enabledCount > 0; | |
| var subtitle = hasEnabledTools | |
| ? $"{enabledCount} / {totalCount} tools enabled" | |
| : "0 tools enabled"; | |
| var description = hasEnabledTools | |
| ? $"You have {enabledCount} out of {totalCount} tools enabled.\n\n" + | |
| "You can manage enabled tools in Window → MCP → Tools. " + | |
| "Disabling unused tools saves LLM context tokens." | |
| : "No tools are currently enabled. The MCP client won't be able to perform any actions.\n\n" + | |
| "To fix this:\n" + | |
| "1. Go to Window → MCP → Tools\n" + | |
| "2. Enable at least one tool by toggling it on"; | |
| var status = hasEnabledTools ? CheckStatus.Success : CheckStatus.Error; | |
| return new StatusCheckItem( | |
| id: "enabled-tools", | |
| title: "MCP Plugin has at least 1 enabled tool", | |
| subtitle: subtitle, | |
| description: description, | |
| status: status | |
| ); |
Comment on lines
+546
to
+553
| if (item.Status == CheckStatus.Error) | ||
| { | ||
| descriptionLabel.style.display = DisplayStyle.Flex; | ||
| } | ||
| else | ||
| { | ||
| descriptionLabel.style.display = DisplayStyle.None; | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Suggested change
| if (item.Status == CheckStatus.Error) | |
| { | |
| descriptionLabel.style.display = DisplayStyle.Flex; | |
| } | |
| else | |
| { | |
| descriptionLabel.style.display = DisplayStyle.None; | |
| } | |
| descriptionLabel.style.display = item.Status == CheckStatus.Error | |
| ? DisplayStyle.Flex | |
| : DisplayStyle.None; |
IvanMurzak
reviewed
Jan 10, 2026
Comment on lines
117
to
126
| public static ClientConfig[] GetAllClientConfigs() | ||
| { | ||
| #if UNITY_EDITOR_WIN | ||
| return GetClientConfigsWindows(); | ||
| #elif UNITY_EDITOR_OSX || UNITY_EDITOR_LINUX | ||
| return GetClientConfigsMacAndLinux(); | ||
| #else | ||
| return GetClientConfigsWindows(); | ||
| #endif | ||
| } |
IvanMurzak
reviewed
Jan 10, 2026
Comment on lines
147
to
152
| private static string GetProjectRootPath() | ||
| { | ||
| return Application.dataPath.EndsWith("/Assets") | ||
| ? Application.dataPath.Substring(0, Application.dataPath.Length - "/Assets".Length) | ||
| : Application.dataPath; | ||
| } |
Owner
There was a problem hiding this comment.
Do we need it? Application.dataPath probably returns the right path
IvanMurzak
reviewed
Jan 10, 2026
|
|
||
| // Check 4: Server to client (pending, doesn't count as passed) | ||
| // Check 5: Client location (if config exists and connected) | ||
| if (GetConfiguredClients().Count > 0 && UnityMcpPlugin.IsConnected.CurrentValue) |
Owner
There was a problem hiding this comment.
Lets save the result of this method into a variable and don't need to call it multiple times
GetConfiguredClients()
IvanMurzak
reviewed
Jan 10, 2026
| var statusChecksLabel = root.Query<Label>("statusChecksLabel").First(); | ||
|
|
||
| // Update status checks count | ||
| void UpdateStatusChecksCount() |
Owner
There was a problem hiding this comment.
Lets make this function not nested.
IvanMurzak
reviewed
Jan 10, 2026
Comment on lines
276
to
278
| // Check 3: Version handshake (if connected) | ||
| if (UnityMcpPlugin.IsConnected.CurrentValue) | ||
| passedCount++; |
Owner
There was a problem hiding this comment.
Version handshake should check version of the server and plugin API.
This is the first batch of fixes addressing code review issues. Not final result. Changes: - McpStatusChecksWindow.cs: Use Interlocked.Increment for thread-safe tool execution counter - MainWindowEditor.ClientConfigure.cs: Add proper error logging instead of silent exception swallowing - MainWindowEditor.ClientConfigure.cs: Remove duplicate GetProjectRootPath() method, use static ProjectRootPath - MainWindowEditor.ClientConfigure.cs: Refactor ConfigureClientsWindows/ConfigureClientsMacAndLinux to use static config methods - MainWindowEditor.CreateGUI.cs: Cache GetConfiguredClients() result in UpdateStatusChecksCount - MainWindowEditor.CreateGUI.cs: Extract local function UpdateStatusChecksCount to class method
Changes: - Add OnToolExecuted event to UnityMcpPlugin (Runtime) for tracking tool executions - Subscribe to OnToolExecuted event in McpStatusChecksWindow to increment counter - Persist tool execution counter using EditorPrefs - Fix UpdateStatusChecksCount to check VersionHandshakeStatus.Compatible - Remove duplicate variable declaration in UpdateStatusChecksCount All status checks now properly use MCP Server APIs: - Version handshake: Uses VersionHandshakeStatus.Compatible from new API - MCP Server connected to MCP Client: Uses VersionHandshakeStatus API - MCP Client location match: Uses CurrentBaseDirectory API - MCP Tool executed: Uses event-based counter with persistence Related: PR #411 (Feature/onboard window) Related: PR #37 (Add Version, BasePath and VersionHandshakeStatus to IMcpPlugin public API)
Replace stubs with proper API integration: - Version handshake: Use VersionHandshakeStatus.Compatible with Major.Minor check - Client location: Use CurrentBaseDirectory API for path comparison - Tool execution: Session-based counter (no persistence) - Fix Observable type and SubscribeToUpdates() structure
# Conflicts: (pick main) # Unity-MCP-Plugin/Assets/root/Editor/Scripts/UI/Window/MainWindowEditor.ClientConfigure.cs # Unity-MCP-Plugin/Assets/root/Editor/UI/uxml/MainWindow.uxml
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
New McpStatusChecksWindow with 7 onboarding status checks:
ConnectionStateToolManagerUI Features:
Status Checks (X/7)counter in MainWindow headerMcpToolsWindow#383838)Files changed:
statusChecksLabelServerVersiontoRemoteMcpManagerHubresponseMcpStatusChecksWindow.NotifyToolExecuted()after each tool call & persist across domain reloads