feat: Add delete button for MCP configuration#718
Merged
Conversation
エディタUI上でMCP設定を作成できるが削除手段がなく、 手動でJSONファイルを編集する必要があった。 「Delete Configuration」ボタンを追加し、uLoopMCPエントリのみを 安全に削除できるようにした（他のMCPサーバー設定は保持）。 - IMcpConfigService に DeleteConfiguration() を追加 - McpConfigRepository にJSON用の削除ロジックを実装 - CodexTomlConfigService にTOML用の削除ロジックを実装 - EditorConfigSection に削除ボタンのUI・バインディングを追加 - 確認ダイアログで誤削除を防止 - JSON/TOML両方の削除処理に対するユニットテストを追加
独自の赤系カラースキームからStop Serverボタンと同じ配色に変更し、 UI全体のトーンを統一した
ボタン配置をConfigure → Open Settings → Deleteの順に変更し、 margin-top: 12pxで視覚的に分離した
独自のbackground-colorとborder-colorを削除しデフォルトスタイルを 継承させることで、他のボタンと同じ幅に揃えた
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a delete-configuration feature: repository and service methods remove uLoopMCP entries from JSON/TOML configs, UI wiring and a confirmation flow to trigger deletion, and new NUnit tests covering TOML and JSON deletion scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant EditorConfigSection
participant McpEditorWindow
participant McpConfigService
participant McpConfigRepository
participant ConfigFile
User->>EditorConfigSection: Click Delete Button
EditorConfigSection->>McpEditorWindow: OnDeleteConfigClicked
McpEditorWindow->>User: Show Confirmation Dialog
alt User Confirms
User-->>McpEditorWindow: Confirm
McpEditorWindow->>McpConfigService: DeleteConfiguration()
McpConfigService->>McpConfigRepository: DeleteULoopMCPEntries(configPath)
McpConfigRepository->>ConfigFile: Load (JSON/TOML)
ConfigFile-->>McpConfigRepository: Return content
McpConfigRepository->>McpConfigRepository: Remove uLoopMCP entries/section
McpConfigRepository->>ConfigFile: Write updated config
McpConfigRepository-->>McpConfigService: Complete
McpConfigService-->>McpEditorWindow: Complete
McpEditorWindow->>EditorConfigSection: Refresh UI
else User Cancels
User-->>McpEditorWindow: Cancel
McpEditorWindow->>McpEditorWindow: No action
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Config/McpConfigRepository.cs">
<violation number="1" location="Packages/src/Editor/Config/McpConfigRepository.cs:324">
P2: Guard against null/non-object `mcpServers` before calling `JObject.FromObject`; current code can throw for valid JSON like `"mcpServers": null` and break deletion.</violation>
</file>
<file name="Assets/Tests/Editor/CodexConfigDeleteTests.cs">
<violation number="1" location="Assets/Tests/Editor/CodexConfigDeleteTests.cs:15">
P2: These tests are coupled to a private implementation detail (`SectionRegex`) and reimplement deletion logic instead of testing `DeleteConfiguration()` behavior. This can miss real regressions in the public method and makes tests brittle to internal refactors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return; | ||
| } | ||
|
|
||
| JObject existingServers = JObject.FromObject(jsonStructure[McpConstants.JSON_KEY_MCP_SERVERS]); |
There was a problem hiding this comment.
P2: Guard against null/non-object mcpServers before calling JObject.FromObject; current code can throw for valid JSON like "mcpServers": null and break deletion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/src/Editor/Config/McpConfigRepository.cs, line 324:
<comment>Guard against null/non-object `mcpServers` before calling `JObject.FromObject`; current code can throw for valid JSON like `"mcpServers": null` and break deletion.</comment>
<file context>
@@ -297,6 +297,62 @@ private Dictionary<string, object> ExtractULoopMCPServers(Dictionary<string, obj
+ return;
+ }
+
+ JObject existingServers = JObject.FromObject(jsonStructure[McpConstants.JSON_KEY_MCP_SERVERS]);
+
+ List<string> keysToRemove = new();
</file context>
Suggested change
| JObject existingServers = JObject.FromObject(jsonStructure[McpConstants.JSON_KEY_MCP_SERVERS]); | |
| if (jsonStructure[McpConstants.JSON_KEY_MCP_SERVERS] is not JObject existingServers) | |
| { | |
| return; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Tests/Editor/CodexConfigDeleteTests.cs`:
- Around line 109-120: The test Should_ReturnUnchanged_WhenNoULoopMCPSection
currently only checks that GetSectionRegex().IsMatch(toml) is false; update it
to also invoke the actual transformation/removal routine that uses
GetSectionRegex (the method under test that removes uLoopMCP sections) with the
toml input and assert the returned string equals the original toml variable,
ensuring no changes were made; keep the existing regex assertion but add
Assert.AreEqual(toml, result, "...") using the same toml sample and reference
the test's local symbols sectionRegex, toml, and the removal function (the
method that applies the sectionRegex) to locate where to call it.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Assets/Tests/Editor/CodexConfigDeleteTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/McpConfigRepositoryDeleteTests.cs.metais excluded by none and included by none
📒 Files selected for processing (11)
Assets/Tests/Editor/CodexConfigDeleteTests.csAssets/Tests/Editor/McpConfigRepositoryDeleteTests.csPackages/src/Editor/Config/CodexTomlConfigService.csPackages/src/Editor/Config/IMcpConfigService.csPackages/src/Editor/Config/McpConfigRepository.csPackages/src/Editor/Config/McpConfigService.csPackages/src/Editor/UI/McpEditorWindow.csPackages/src/Editor/UI/UIToolkit/Components/EditorConfigSection.csPackages/src/Editor/UI/UIToolkit/McpEditorWindow.ussPackages/src/Editor/UI/UIToolkit/McpEditorWindow.uxmlPackages/src/Editor/UI/UIToolkit/McpEditorWindowUI.cs
Replace regex-only assertion with actual SimulateDelete call to ensure the transformation path preserves content intact
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
1 participant
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.
Summary
Changes
Core logic
IMcpConfigService: AddedDeleteConfiguration()methodMcpConfigRepository: AddedDeleteULoopMCPEntries()for JSON config deletionMcpConfigService: ImplementedDeleteConfiguration()delegating to repositoryCodexTomlConfigService: ImplementedDeleteConfiguration()using regex-based TOML section removalUI
.mcp-button--deletestyle with red text to indicate destructive actionTests
McpConfigRepositoryDeleteTests: 5 tests covering JSON deletion (preserve other servers, handle port-suffixed keys, missing file, no-op cases)CodexConfigDeleteTests: 5 tests covering TOML deletion (section at beginning/middle/end, only section, no match)Test plan
Summary by cubic
Adds a Delete Configuration button in MCP settings to remove uLoopMCP entries without editing JSON/TOML by hand. It confirms before deleting and leaves other MCP servers untouched.
New Features
Bug Fixes
Written for commit 33e158c. Summary will update on new commits.
MCP Configuration Delete Feature
Overview
This PR adds a "Delete Configuration" button to the MCP settings UI, enabling users to remove uLoopMCP entries from configuration files without manual JSON/TOML editing. The feature includes a confirmation dialog before deletion and preserves other MCP server configurations. The button is visible only when a configuration exists and is removed after deletion.
Architecture & Implementation
Core Service Layer
The deletion feature follows the existing service/repository pattern:
classDiagram class IMcpConfigService { +DeleteConfiguration() void } class McpConfigService { +DeleteConfiguration() void -FindExistingULoopMCPConfigurationKey() string } class CodexTomlConfigService { +DeleteConfiguration() void } class McpConfigRepository { +DeleteULoopMCPEntries(configPath) void } IMcpConfigService <|.. McpConfigService IMcpConfigService <|.. CodexTomlConfigService McpConfigService --> McpConfigRepositoryUI Layer
classDiagram class McpEditorWindow { -DeleteEditorConfiguration() void } class McpEditorWindowUI { +OnDeleteConfigClicked } class EditorConfigSection { +OnDeleteConfigClicked +UpdateDeleteButton(data) void } McpEditorWindow --> McpEditorWindowUI McpEditorWindowUI --> EditorConfigSection EditorConfigSection --> McpEditorWindowEvent flow: EditorConfigSection -> McpEditorWindowUI -> McpEditorWindow -> confirmation dialog -> IMcpConfigService.DeleteConfiguration() -> repository/service logic -> UI refresh.
Test Coverage
JSON Configuration Tests (McpConfigRepositoryDeleteTests)
New NUnit test fixture McpConfigRepositoryDeleteTests with cases:
Tests create temporary JSON files, invoke DeleteULoopMCPEntries, and assert file contents/behavior.
TOML Configuration Tests (CodexConfigDeleteTests)
New NUnit test fixture CodexConfigDeleteTests with cases:
Tests simulate deletion by accessing the private static SectionRegex on CodexTomlConfigService via reflection and verify correct removal/normalization behavior.
Key Behaviors
Files Changed (high level)