fix(ui): default the per-client setup foldout to expanded so Unregister is visible#1152
Merged
Scriptwonder merged 5 commits intoMay 24, 2026
Conversation
Community report: "the Unregister button was removed in 9.7.0 — it did a wrong registration with stdio transport and now it's difficult to re-register; something is stuck on stdio and I can't switch to local." The button wasn't actually removed — the 9.7.0 UI shuffle put the single-client Configure (which toggles to Unregister for CLI-based clients like Claude Code) inside a "Per-client setup" foldout that defaulted to collapsed. With Configure All sitting prominently above it, the foldout looked like terminal styling rather than the entry point to manual per-client management, so users who needed to wipe a bad stdio registration and re-add with HTTP couldn't find the button. Flip the default to expanded in both UXML and the EditorPrefs fallback. The state still persists per-user, so anyone who explicitly collapses it keeps that preference — only the never-touched default changes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPer-client details foldout now defaults expanded. Interface and base configurator add Unregister; JsonFileMcpConfigurator shows Configure/Unregister label and removes unityMCP from JSON when unregistering. Configure button toggles per-client unregister/configure, and bulk configure clears cached statuses. ChangesClient Details Foldout & JSON Configurator
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Unity Editor “Client Configuration” UI so the “Per-client setup” foldout defaults to expanded, making the per-client Configure/Unregister action immediately visible (addressing confusion introduced by the 9.7.0 UI reshuffle while still respecting persisted user preference via EditorPrefs).
Changes:
- Default “Per-client setup” foldout to expanded in the UXML (
value="true"). - Change the
EditorPrefsfallback default forEditorPrefKeys.ClientDetailsFoldoutOpenfromfalsetotrueso first-time users see the expanded state. - Update inline comment to document the rationale and user impact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.uxml | Sets the foldout’s markup default to expanded so the per-client controls are visible by default. |
| MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs | Changes the persisted-state fallback default to expanded and documents why; still preserves per-user preference when explicitly toggled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… toggle) The 9.7.0 community complaint about "Unregister was removed" was actually two stacked issues: the foldout default hid the existing toggle (fixed in the previous commit), and the toggle never existed for JSON-based clients in the first place. Only ClaudeCliMcpConfigurator overrode GetConfigureActionLabel and routed Configure() through register/unregister based on status; every JsonFileMcpConfigurator-derived client (Antigravity, Antigravity IDE, Claude Desktop, Cursor, Windsurf, Kiro, Trae, VS Code, the various Copilot configs, etc.) always showed "Configure" and re-wrote the file on every click — there was no way to actually clean a bad entry out of the JSON without manually editing the file. Move the toggle logic into JsonFileMcpConfigurator so every JSON client inherits it: - GetConfigureActionLabel() flips to "Unregister" when status == Configured. - Configure() routes to a new UnregisterFromConfig() helper in that case, which parses the file and removes the unityMCP entry from mcpServers (standard layout) or servers / mcp.servers (VS Code layout). The file itself is preserved so the user's other MCP servers stay intact, and the cleanup re-uses the same JsonConvert / Newtonsoft path the existing CheckStatus / ConfigJsonBuilder code already uses. - Status flips to NotConfigured after a successful Unregister; the next CheckStatus reads the file, sees mcpServers no longer contains unityMCP, and reports MissingConfig — so the startup auto-rewrite does not silently undo the user's Unregister. Codex (TOML) clients still don't get the toggle in this commit — the TOML helpers don't have a remove path and adding one would risk breaking the file shape. Codex is a small enough surface that hand-editing is acceptable; a proper Codex remove can be a follow-up.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
410-413: ⚡ Quick winPreserve the original exception as
InnerExceptionwhen rethrowing.Line 412 currently drops stack/context from the original failure. Chain the exception to keep actionable diagnostics.
Proposed fix
- catch (Exception ex) - { - throw new InvalidOperationException($"Failed to unregister: {ex.Message}"); - } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed to unregister: {ex.Message}", ex); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs` around lines 410 - 413, In the catch block inside McpClientConfiguratorBase (the catch(Exception ex) that currently throws new InvalidOperationException($"Failed to unregister: {ex.Message}")), preserve the original exception by passing ex as the InnerException when rethrowing; replace the current throw with creating an InvalidOperationException that includes a clear message (e.g. "Failed to unregister.") and the caught exception as the innerException so the original stack/context is retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 410-413: In the catch block inside McpClientConfiguratorBase (the
catch(Exception ex) that currently throws new InvalidOperationException($"Failed
to unregister: {ex.Message}")), preserve the original exception by passing ex as
the InnerException when rethrowing; replace the current throw with creating an
InvalidOperationException that includes a clear message (e.g. "Failed to
unregister.") and the caught exception as the innerException so the original
stack/context is retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43bd6000-4a62-4247-85a5-1263a2991262
📒 Files selected for processing (1)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs
…ent-setup-visibility
…r via UI User-reported regression from the previous commit on this PR: "Configure All Detected Clients does not actually configure all IDEs; after I click Configure each individual IDE still shows missing configs, and if one is configured Configure All resets it." Root cause was the toggle I'd put inside JsonFileMcpConfigurator.Configure(): when ConfigureAllDetectedClients walked the registry and called configurator.Configure() on each one, any client whose status was already Configured took the Unregister branch — so the bulk action wiped every already-configured JSON client instead of refreshing it. (The same trap existed for ClaudeCli even before this PR, but Claude Code's CLI registration path is rarely hit through the bulk button so nobody had reported it.) Fix: move the Configure↔Unregister toggle out of the configurator and into the UI handler. The configurator API now has two clearly-split operations: - IMcpClientConfigurator.Configure(): always idempotent-write. Safe to call repeatedly. This is what ConfigureAllDetectedClients calls and what makes the "refresh transport / server version drift" use case work without resetting anything. - IMcpClientConfigurator.Unregister(): removes UnityMCP from this client's config. McpClientConfiguratorBase ships a no-op default; JsonFileMcpConfigurator overrides with the JObject-parse + remove path that used to be the private UnregisterFromConfig helper. Codex's TOML still has no remove path so it inherits the no-op, matching the previous commit's stance. UI per-client click (OnConfigureClicked) now reads client.Status and routes: Configured → client.Unregister(); else → MCPServiceLocator.Client.ConfigureClient(client). The button label toggle from GetConfigureActionLabel is preserved — it's purely informational and stays consistent with the routed action. Secondary fix in OnConfigureAllClientsClicked: clear the lastStatusChecks cache after the bulk run so dropdown-switching to any non-currently-selected client immediately reads its post-bulk status from disk instead of waiting out the 45-second throttle. This was the "after clicking Configure All each individual IDE still shows missing configs" symptom even when the writes had actually succeeded. ClaudeCli's existing internal toggle is preserved (its async path in ConfigureClaudeCliAsync handles the Configuring…/Unregistering… UX itself, and OnConfigureClicked still early-routes to that helper).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 49-51: McpClientConfiguratorBase defines a public virtual
Unregister(), but ClaudeCliMcpConfigurator currently defines a private
Unregister() which hides rather than overrides it; change
ClaudeCliMcpConfigurator.Unregister to match the base signature (public override
void Unregister()) so calls dispatched via the base type/interface invoke
Claude’s implementation, and remove the private/hiding method to avoid
duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 049958b2-f648-42d3-8bc3-1df94f4510b7
📒 Files selected for processing (3)
MCPForUnity/Editor/Clients/IMcpClientConfigurator.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
Two review items from CodeRabbit on PR CoplayDev#1152: 1) ClaudeCliMcpConfigurator.Unregister was declared `private` while the base class defines `public virtual void Unregister()`. Per C# rules that hides the base rather than overriding it — so a polymorphic IMcpClientConfigurator.Unregister() call on a ClaudeCli instance would land on the base's no-op default and the existing `claude mcp remove --scope ...` logic would be unreachable through the interface. Promote it to `public override` so the interface contract resolves correctly; the body is unchanged. 2) JsonFileMcpConfigurator.Unregister rethrew with only the message string, dropping the original stack/context. Pass `ex` through as InnerException so diagnostics are preserved.
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
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.
Community reports:
The Unregister button wasn't actually removed — the 9.7.0 UI reshuffle put the single-client Configure button (which toggles to "Unregister" for CLI-based clients like Claude Code) inside a "Per-client setup" foldout that defaulted to collapsed. With "Configure All Detected Clients" sitting prominently green above it, the foldout reads more like a section divider than the entry point to manual per-client management. Users who need to wipe a bad stdio registration and re-add with HTTP couldn't find it.
Flip the default to expanded in both the UXML markup and the
EditorPrefsfallback. State still persists per-user viaEditorPrefKeys.ClientDetailsFoldoutOpen, so anyone who explicitly collapses it keeps that preference — only the never-touched default changes, which is exactly the cohort the community report is from.Summary by CodeRabbit
Improvements
New Features