Remove old UI and do lots of cleanup #340
Conversation
Users now have the new UI alone, less confusing and more predictable
The docs have 3.12. However, feature wise it seems that 3.11 is required
…list The recent changes should close CoplayDev#311
… lifecycle manager Duplicate code for pretty much no reason, as they both initialized there was a small chance of a race condition as well. Consolidating made sense here
WalkthroughRemoved legacy server config/model and multiple Editor windows/utilities; added PackageLifecycleManager and PlatformService; centralized menu items; bumped Python requirement to 3.11; consolidated config/path and atomic-write helpers; changed TOML upsert to always write and inject Windows SystemRoot when applicable; updated tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Unity Editor
participant PLM as PackageLifecycleManager
participant Prefs as EditorPrefs
participant Installer as ServerInstaller
Editor->>PLM: schedule delayed lifecycle check
activate PLM
PLM->>Prefs: read per-version keys
alt missing or legacy detected
Prefs-->>PLM: key missing / legacy present
PLM->>Installer: EnsureServerInstalled()
activate Installer
Installer-->>PLM: install success/failure
deactivate Installer
PLM->>Prefs: set version key / store install error / cleanup legacy prefs
else already installed
Prefs-->>PLM: key present
Note right of PLM: skip install
end
deactivate PLM
sequenceDiagram
participant User as User
participant Menu as MCPForUnityMenu
participant SW as SetupWizard
participant Sync as PythonToolSyncProcessor
participant Win as MCPForUnityEditorWindow
User->>Menu: Click "Setup Wizard"
Menu->>SW: ShowSetupWizard()
User->>Menu: Click "Sync Python Tools"
Menu->>Sync: ManualSync()
User->>Menu: Click "Open MCP Window"
Menu->>Win: ShowWindow()
Note over Menu: central menu routes commands to helpers
sequenceDiagram
participant Codex as CodexConfigHelper
participant Plat as PlatformService
participant TOML as TOML file
Codex->>Plat: IsWindows()
alt Windows
Plat-->>Codex: true
Codex->>Plat: GetSystemRoot()
Plat-->>Codex: "C:\\Windows" or env
Codex->>TOML: preserve existing env, ensure SystemRoot present
else Non-Windows
Plat-->>Codex: false
Codex->>TOML: preserve existing env (no SystemRoot added)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
Plugin is working well on my end, but I'll test on Windows before releasing this version |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
11-11: Python version badge inconsistent with prerequisites.Line 11 shows
Python-3.12but line 83 updates the prerequisites toPython: Version 3.11 or newer. Per the PR objectives to set the minimum to 3.11, the badge on line 11 should also be updated to reflectPython-3.11for consistency.Apply this diff to fix the inconsistency:
-[](https://www.python.org) +[](https://www.python.org)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
124-139: Handle python --version on stderr to avoid false negatives.Many Python builds print version to stderr. Failing to read it causes missed detections.
Apply:
- string output = process.StandardOutput.ReadToEnd().Trim(); + string stdout = process.StandardOutput.ReadToEnd().Trim(); + string stderr = process.StandardError.ReadToEnd().Trim(); + string output = string.IsNullOrEmpty(stdout) ? stderr : stdout; process.WaitForExit(5000);Also consider increasing robustness by checking both streams before version parsing.
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
54-73: Implement non-destructive merge for unityMCP to preserve user customization.The review identifies a real issue:
UpsertCodexServerBlockat line 67 performs a destructive replacement via direct assignment instead of merging. This loses any custom keys or environment variables a user may have added to the existingunityMCPtable. The suggested refactor is correct and required.Current behavior discards user edits on every update. The proposed merge logic—iterating existing keys and preserving those not set by
CreateUnityMcpTable—is sound and maintains backward compatibility while preventing data loss.Additionally, existing tests only verify that the new table contains expected keys; they do not cover preservation of custom keys. Add test(s) to
CodexConfigHelperTests.csthat create a TOML with a pre-existingunityMCPcontaining custom env vars or flags, then verify those keys survive after callingUpsertCodexServerBlock.
🧹 Nitpick comments (11)
MCPForUnity/Editor/Setup/SetupWizardWindow.cs (1)
123-123: Consider using direct emoji characters for consistency.The warning symbol uses Unicode escape sequence
\u26A0here (lines 123, 281), while other parts of the code use direct emoji characters like "✓", "✗", "🎉", and "🚨". For consistency and readability, consider replacing\u26A0with the direct warning emoji "⚠️ " or "⚠".Apply this diff to improve consistency:
- "\u26A0 Missing Dependencies: MCP for Unity requires Python 3.11+ and UV package manager to function properly.", + "⚠ Missing Dependencies: MCP for Unity requires Python 3.11+ and UV package manager to function properly.",And:
- "\u26A0 Skipping setup will leave MCP for Unity non-functional!\n\n" + + "⚠ Skipping setup will leave MCP for Unity non-functional!\n\n" +Also applies to: 281-281
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
29-39: Consider adding Python 3.12+ framework paths for better detection coverage.The candidate paths now only check for Python 3.11 in the Framework directory, but users installing Python 3.12 or 3.13 via the official python.org installer would have installations at
/Library/Frameworks/Python.framework/Versions/3.12/or.../3.13/. While the code falls back to PATH resolution, explicitly checking these paths would improve detection reliability.Consider restoring the 3.12 and 3.13 paths for forward compatibility:
var candidates = new[] { "python3", "python", "/usr/bin/python3", "/usr/local/bin/python3", "/opt/homebrew/bin/python3", "/Library/Frameworks/Python.framework/Versions/3.13/bin/python3", "/Library/Frameworks/Python.framework/Versions/3.12/bin/python3", "/Library/Frameworks/Python.framework/Versions/3.11/bin/python3" };MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (2)
29-43: Include ProgramFiles\Python311 in candidates.You probe ProgramFiles for 3.13 and 3.12 but miss 3.11. Add it for completeness.
Apply:
Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), "Python312", "python.exe") + , + Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), + "Python311", "python.exe")
172-180: Don’t assume the first “where” result is usable; iterate and avoid File.Exists gate.The first hit may be the WindowsApps alias (File.Exists returns false). Iterate all lines and validate via TryValidatePython instead of requiring File.Exists.
Apply:
- if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) - { - // Take the first result - var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); - if (lines.Length > 0) - { - fullPath = lines[0].Trim(); - return File.Exists(fullPath); - } - } + if (process.ExitCode == 0 && !string.IsNullOrEmpty(output)) + { + var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); + foreach (var line in lines) + { + var candidate = line.Trim(); + // Defer validation to TryValidatePython (handles aliases) + if (TryValidatePython(candidate, out _, out _)) + { + fullPath = candidate; + return true; + } + } + }MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
139-151: Also read stderr for python --version.Some distros print version to stderr; current logic misses them. Mirror Windows fix.
Apply:
- string output = process.StandardOutput.ReadToEnd().Trim(); + string stdout = process.StandardOutput.ReadToEnd().Trim(); + string stderr = process.StandardError.ReadToEnd().Trim(); + string output = string.IsNullOrEmpty(stdout) ? stderr : stdout; process.WaitForExit(5000);MCPForUnity/Editor/Services/PlatformService.cs (1)
14-17: Prefer RuntimeInformation (Unity-friendly) for OS detection.Environment.OSVersion can be unreliable; RuntimeInformation.IsOSPlatform(OSPlatform.Windows) is clearer.
Apply:
- return Environment.OSVersion.Platform == PlatformID.Win32NT; + return System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform( + System.Runtime.InteropServices.OSPlatform.Windows);TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
131-177: Great coverage of Windows env; add preservation test.Add a case where existing unityMCP.env contains extra keys (e.g., PATH, CUSTOM) and assert they survive Build/Upsert on Windows with SystemRoot added/updated. This will guard against regressions if merge logic changes.
218-273: Upsert preservation test recommended.Similarly, seed config.toml with an existing unityMCP block containing custom keys and verify Upsert keeps them while updating command/args/env.SystemRoot.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PackageLifecycleManagerTests.cs (1)
59-165: Exercise the real lifecycle API, not just keys.If PackageLifecycleManager exposes public methods/events, add tests that call them and assert EditorPrefs changes, instead of manipulating keys directly. Strengthens signal and reduces coupling to key names.
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
165-176: Consider removing the placeholder method.
CleanupOldVersionKeyscontains only an empty try-catch block with a comment. Since Unity doesn't provide key enumeration and the method serves no current purpose, consider either removing it entirely or adding a clear TODO comment if future implementation is planned.Apply this diff to remove the dead code:
- private static void CleanupOldVersionKeys(string currentVersion) - { - try - { - // Get all EditorPrefs keys that start with our version prefix - // Note: Unity doesn't provide a way to enumerate all keys, so we can only - // clean up known legacy keys. Future versions will be cleaned up when - // a newer version runs. - // This is a best-effort cleanup. - } - catch { } - } -Or if you plan to implement this later, make it clearer:
private static void CleanupOldVersionKeys(string currentVersion) { - try - { - // Get all EditorPrefs keys that start with our version prefix - // Note: Unity doesn't provide a way to enumerate all keys, so we can only - // clean up known legacy keys. Future versions will be cleaned up when - // a newer version runs. - // This is a best-effort cleanup. - } - catch { } + // TODO: Implement version key cleanup if Unity provides key enumeration in the future. + // Note: Unity doesn't provide a way to enumerate all EditorPrefs keys. + // For now, legacy keys are cleaned up in CleanupLegacyPrefs(). }MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (1)
179-186: Consider extracting the hard-coded menu path to avoid coupling.Line 184 contains a hard-coded menu path that must match the MenuItem attribute in MCPForUnityMenu.cs (line 60). If the menu path changes, this string won't update automatically, potentially causing the validation to fail silently.
Consider one of these solutions:
Option 1: Define a shared constant in MCPForUnityMenu
In MCPForUnityMenu.cs, add:
public const string AutoSyncMenuPath = "Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools";Then use it in both places:
- [MenuItem("Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools", priority = 101)] + [MenuItem(AutoSyncMenuPath, priority = 101)] public static void ToggleAutoSync()And in PythonToolSyncProcessor.cs:
- Menu.SetChecked("Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools", IsAutoSyncEnabled()); + Menu.SetChecked(MCPForUnityMenu.AutoSyncMenuPath, IsAutoSyncEnabled());Option 2: Pass the menu path as a parameter
Modify the validation method to accept the menu path:
- public static bool ToggleAutoSyncValidate() + public static bool ToggleAutoSyncValidate(string menuPath) { - Menu.SetChecked("Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools", IsAutoSyncEnabled()); + Menu.SetChecked(menuPath, IsAutoSyncEnabled()); return true; }Then update MCPForUnityMenu.cs:
+ private const string AutoSyncMenuPath = "Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools"; + - [MenuItem("Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools", priority = 101)] + [MenuItem(AutoSyncMenuPath, priority = 101)] public static void ToggleAutoSync() - [MenuItem("Window/MCP For Unity/Tool Sync/Auto-Sync Python Tools", true, priority = 101)] + [MenuItem(AutoSyncMenuPath, true, priority = 101)] public static bool ToggleAutoSyncValidate() { - return PythonToolSyncProcessor.ToggleAutoSyncValidate(); + return PythonToolSyncProcessor.ToggleAutoSyncValidate(AutoSyncMenuPath); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
MCPForUnity/UnityMcpServer~/src/uv.lockis excluded by!**/*.lockdocs/images/coplay-logo.pngis excluded by!**/*.pngdocs/images/logo.pngis excluded by!**/*.pngdocs/images/readme_ui.pngis excluded by!**/*.pngdocs/images/v5_01_uninstall.pngis excluded by!**/*.pngdocs/images/v5_02_install.pngis excluded by!**/*.pngdocs/images/v5_03_open_mcp_window.pngis excluded by!**/*.pngdocs/images/v5_04_rebuild_mcp_server.pngis excluded by!**/*.pngdocs/images/v5_05_rebuild_success.pngis excluded by!**/*.pngdocs/images/v6_2_create_python_tools_asset.pngis excluded by!**/*.pngdocs/images/v6_2_python_tools_asset.pngis excluded by!**/*.pngdocs/images/v6_new_ui_asset_store_version.pngis excluded by!**/*.pngdocs/images/v6_new_ui_dark.pngis excluded by!**/*.pngdocs/images/v6_new_ui_light.pngis excluded by!**/*.png
📒 Files selected for processing (42)
MCPForUnity/Editor/Data/DefaultServerConfig.cs(0 hunks)MCPForUnity/Editor/Dependencies/DependencyManager.cs(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs(2 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs(3 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs(2 hunks)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs(2 hunks)MCPForUnity/Editor/Helpers/PackageDetector.cs(0 hunks)MCPForUnity/Editor/Helpers/PackageInstaller.cs(0 hunks)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs(1 hunks)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs.meta(1 hunks)MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs(2 hunks)MCPForUnity/Editor/Helpers/Vector3Helper.cs(0 hunks)MCPForUnity/Editor/MCPForUnityMenu.cs(1 hunks)MCPForUnity/Editor/MCPForUnityMenu.cs.meta(1 hunks)MCPForUnity/Editor/Models/ServerConfig.cs(0 hunks)MCPForUnity/Editor/Models/ServerConfig.cs.meta(0 hunks)MCPForUnity/Editor/Services/IPlatformService.cs(1 hunks)MCPForUnity/Editor/Services/IPlatformService.cs.meta(1 hunks)MCPForUnity/Editor/Services/MCPServiceLocator.cs(5 hunks)MCPForUnity/Editor/Services/PlatformService.cs(1 hunks)MCPForUnity/Editor/Services/PlatformService.cs.meta(1 hunks)MCPForUnity/Editor/Setup/SetupWizard.cs(0 hunks)MCPForUnity/Editor/Setup/SetupWizardWindow.cs(3 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs.meta(1 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs(0 hunks)MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs.meta(0 hunks)MCPForUnity/Editor/Windows/ManualConfigEditorWindow.cs(0 hunks)MCPForUnity/Editor/Windows/ManualConfigEditorWindow.cs.meta(0 hunks)MCPForUnity/Editor/Windows/VSCodeManualSetupWindow.cs(0 hunks)MCPForUnity/Editor/Windows/VSCodeManualSetupWindow.cs.meta(0 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)README.md(10 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PackageLifecycleManagerTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PackageLifecycleManagerTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows.meta(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs.meta(0 hunks)docs/CUSTOM_TOOLS.md(4 hunks)docs/README-DEV.md(12 hunks)docs/v5_MIGRATION.md(2 hunks)docs/v6_NEW_UI_CHANGES.md(2 hunks)
💤 Files with no reviewable changes (16)
- MCPForUnity/Editor/Windows/VSCodeManualSetupWindow.cs.meta
- MCPForUnity/Editor/Models/ServerConfig.cs.meta
- MCPForUnity/Editor/Data/DefaultServerConfig.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs.meta
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/ManualConfigJsonBuilderTests.cs
- MCPForUnity/Editor/Windows/ManualConfigEditorWindow.cs.meta
- MCPForUnity/Editor/Models/ServerConfig.cs
- MCPForUnity/Editor/Helpers/Vector3Helper.cs
- MCPForUnity/Editor/Setup/SetupWizard.cs
- MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs.meta
- MCPForUnity/Editor/Helpers/PackageInstaller.cs
- MCPForUnity/Editor/Helpers/PackageDetector.cs
- MCPForUnity/Editor/Windows/ManualConfigEditorWindow.cs
- MCPForUnity/Editor/Windows/VSCodeManualSetupWindow.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows.meta
- MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
PR: CoplayDev/unity-mcp#316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/MCPForUnityMenu.cs.metaREADME.md
🧬 Code graph analysis (8)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
IsWindows(25-25)GetSystemRoot(26-26)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
HasEmbeddedServer(817-820)
MCPForUnity/Editor/Services/PlatformService.cs (2)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
IsWindows(25-25)GetSystemRoot(26-26)
MCPForUnity/Editor/Dependencies/DependencyManager.cs (3)
MCPForUnity/Editor/Dependencies/PlatformDetectors/LinuxPlatformDetector.cs (1)
GetPythonInstallUrl(76-79)MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (1)
GetPythonInstallUrl(78-81)MCPForUnity/Editor/Dependencies/PlatformDetectors/WindowsPlatformDetector.cs (1)
GetPythonInstallUrl(82-85)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPForUnity/Editor/Services/PlatformService.cs (1)
PlatformService(8-30)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-76)MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
IsWindows(25-25)GetSystemRoot(26-26)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (5)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)MCPForUnity/Editor/Services/MCPServiceLocator.cs (3)
MCPServiceLocator(8-76)Reset(56-75)Register(33-51)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(15-221)BuildCodexServerBlock(41-52)TomlTable(103-124)TomlTable(129-152)UpsertCodexServerBlock(54-73)UnityMcpBridge/Editor/External/Tommy.cs (8)
TomlTable(421-560)TomlTable(648-852)TomlTable(1668-1757)TomlTable(1780-1784)TOML(1776-1785)TryGetNode(93-97)TryGetNode(448-448)TomlString(168-193)
MCPForUnity/Editor/MCPForUnityMenu.cs (2)
MCPForUnity/Editor/Setup/SetupWizard.cs (2)
ShowSetupWizard(69-80)SetupWizard(21-29)MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (5)
ReimportPythonFiles(144-160)PythonToolSyncProcessor(22-32)ManualSync(165-169)ToggleAutoSync(174-177)ToggleAutoSyncValidate(182-186)
🪛 LanguageTool
docs/README-DEV.md
[grammar] ~210-~210: Ensure spelling is correct
Context: ...sible. To run it - Trigger: In GitHun "Actions" for the repo, trigger `workfl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/README-DEV.md
210-210: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (32)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs.meta (1)
2-2: No issues found; GUID change is safe.MCPForUnityEditorWindow.cs remains in use (called by MCPForUnityMenu and SetupWizardWindow), and no code contains hardcoded references to the old GUID. The GUID update is consistent with metadata regeneration. The asset bundle field reformatting is a non-functional change. The .meta file modification is appropriate for this scenario.
MCPForUnity/Editor/Setup/SetupWizardWindow.cs (4)
22-25: LGTM! Simplified wizard flow improves user experience.The reduction from 3 steps to 2 steps (removing the "Configure" step) streamlines the setup wizard to focus solely on dependency management, while client configuration is appropriately moved to a dedicated window accessible via the "Client Settings" button (line 176-179). This separation of concerns aligns well with the PR's cleanup objectives.
Also applies to: 51-55
123-123: LGTM! Python 3.11+ requirement correctly reflected.The warning message accurately reflects the updated minimum Python version requirement, aligning with the PR objective to standardize on Python 3.11+.
148-209: LGTM! Well-structured completion step with good UX.The
DrawCompleteStep()implementation effectively handles both success and failure scenarios:
- Smart caching (2-second threshold) prevents redundant dependency checks during repaints
- Clear visual feedback with success/error states
- Helpful navigation with links to documentation and client settings
- Graceful handling of incomplete setup with back navigation
262-313: LGTM! Footer navigation logic is solid.The navigation implementation correctly handles the simplified 2-step flow:
- Back button properly disabled on first step
- Skip confirmation dialog prevents accidental dismissal with clear warnings
- Next/Done button logic appropriately marks setup as completed
- State management calls (
MarkSetupDismissed(),MarkSetupCompleted()) are placed correctlydocs/v6_NEW_UI_CHANGES.md (1)
5-5: Image asset paths updated correctly.The documentation asset references have been consistently updated to use the new
docs/images/directory structure. No issues detected.Also applies to: 8-8, 59-59
docs/v5_MIGRATION.md (1)
18-18: Image asset paths updated correctly.Migration guide image references have been consistently updated to the new
images/directory structure across all five locations.Also applies to: 27-27, 34-34, 36-36, 38-38
docs/CUSTOM_TOOLS.md (1)
24-24: Image asset paths updated correctly.Documentation image references have been properly updated to the new
images/directory structure.Also applies to: 65-65
README.md (2)
1-1: Asset paths and new image references updated correctly.Logo and new UI image paths have been properly updated to the new
docs/images/directory structure. The addition of the readme UI image and Coplay logo placement looks good.Also applies to: 21-21, 364-364
39-39: New Tools and Resources sections well-organized.The addition of the new "Tools" and "Resources" detail sections, the expanded tool list (including
run_test), and the updated Python version requirement to 3.11 are all clear and well-structured. The documentation improvements align well with the PR objectives.Also applies to: 54-54, 58-65, 83-83
MCPForUnity/Editor/MCPForUnityMenu.cs.meta (1)
2-2: LGTM: Routine Unity meta file update.The GUID update is standard for Unity asset reorganization, consistent with the menu consolidation work in this PR.
MCPForUnity/Editor/Dependencies/DependencyManager.cs (1)
129-129: LGTM: Consistent with Python 3.11+ requirement.The recommendation message correctly reflects the new minimum Python version enforced throughout the codebase.
MCPForUnity/Editor/Services/PlatformService.cs.meta (1)
2-2: LGTM: Meta file for new platform service.Standard Unity metadata for the newly introduced PlatformService infrastructure.
MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
6-6: LGTM: Python minimum version correctly updated.The requires-python constraint now enforces Python 3.11+ as intended, aligning with the PR's standardization objective.
MCPForUnity/Editor/Dependencies/PlatformDetectors/MacOSPlatformDetector.cs (2)
67-68: LGTM: Error message correctly updated.The error message now accurately reflects the Python 3.11+ requirement.
146-149: LGTM: Version validation correctly enforces Python 3.11+ minimum.The validation logic properly checks for Python 3.11+ while future-proofing for Python 4+.
MCPForUnity/Editor/Services/MCPServiceLocator.cs (3)
17-17: LGTM: Platform service correctly integrated into service locator.The private field and public property follow the established pattern used by other services in the locator.
Also applies to: 26-26
49-50: LGTM: Platform service registration follows existing pattern.The registration logic for
IPlatformServiceis consistent with other service registrations in the method.
65-65: LGTM: Platform service cleanup properly handled.Disposal and reset logic for the platform service matches the cleanup pattern used for all other services.
Also applies to: 74-74
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs.meta (1)
2-2: LGTM: Meta file for new lifecycle manager.Standard Unity metadata for the newly introduced PackageLifecycleManager, which consolidates package lifecycle logic as described in the PR objectives.
MCPForUnity/Editor/Services/IPlatformService.cs.meta (1)
2-2: LGTM: Meta file for new platform service interface.Standard Unity metadata for the newly introduced IPlatformService, which abstracts platform detection and Windows-specific environment access.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/PackageLifecycleManagerTests.cs.meta (1)
1-11: Meta file present and well-formed.GUID and MonoImporter defaults look fine.
MCPForUnity/Editor/Services/IPlatformService.cs (1)
6-19: Interface shape is minimal and clear.Good separation; enables test doubles.
MCPForUnity/Editor/MCPForUnityMenu.cs (3)
1-34: LGTM! Clean centralized menu structure.The menu facade pattern effectively consolidates editor commands, and the keyboard shortcut for the main window is a nice touch.
39-56: LGTM! Tool sync menu items properly structured.The delegation to PythonToolSyncProcessor is clean and priorities are logically ordered.
57-74: LGTM! Toggle and validation correctly implemented.The validation method correctly uses the
trueparameter to indicate it's a validation callback, and both methods share the same menu path and priority as required by Unity's menu system.MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (5)
1-23: LGTM! Proper initialization pattern.Using
EditorApplication.delayCallfor lifecycle checks is the correct approach to ensure Unity is fully loaded before running package management logic.
51-98: Error handling strategy prevents retry loops.The implementation marks the version as handled even on installation failure (lines 86-91) to prevent repeated failures on every Unity startup. This is a reasonable idempotency strategy, though it means transient errors won't automatically retry. Users can manually recover via the UI as indicated in the error message.
100-133: Version detection with graceful fallbacks.The methods use try-catch with empty catch blocks (lines 112, 131) to provide fallback behavior. While this swallows exceptions, the fallback to "unknown" version string (line 132) ensures the system remains functional even when version detection fails.
135-163: LGTM! Robust legacy detection.The nested try-catch blocks ensure the legacy detection is resilient to filesystem access issues, which is appropriate for compatibility checks across different environments.
178-203: LGTM! Legacy preference cleanup is appropriate.The method correctly cleans up known obsolete EditorPrefs keys with proper error handling for each key deletion.
MCPForUnity/Editor/Helpers/PythonToolSyncProcessor.cs (1)
142-177: LGTM! MenuItem removal and comment updates are consistent.The methods remain public for delegation from MCPForUnityMenu, and the updated comments appropriately describe the direct actions rather than menu behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/README-DEV.md (1)
210-214: Markdown list indentation still requires fixing.This is the same issue flagged in the prior review: the unordered list items on lines 210–214 have incorrect indentation (currently 1 space before each
-; should be 0 per Markdown standards). The typo "GitHun" → "GitHub" has been corrected, but the indentation needs to be adjusted.Apply this diff to fix the indentation:
-***To run it*** - - Trigger: In GitHub "Actions" for the repo, trigger `workflow dispatch` (`Claude NL/T Full Suite (Unity live)`). - - Image: `UNITY_IMAGE` (UnityCI) pulled by tag; the job resolves a digest at runtime. Logs are sanitized. - - Execution: single pass with immediate per‑test fragment emissions (strict single `<testcase>` per file). A placeholder guard fails fast if any fragment is a bare ID. Staging (`reports/_staging`) is promoted to `reports/` to reduce partial writes. - - Reports: JUnit at `reports/junit-nl-suite.xml`, Markdown at `reports/junit-nl-suite.md`. - - Publishing: JUnit is normalized to `reports/junit-for-actions.xml` and published; artifacts upload all files under `reports/`. +***To run it*** +- Trigger: In GitHub "Actions" for the repo, trigger `workflow dispatch` (`Claude NL/T Full Suite (Unity live)`). +- Image: `UNITY_IMAGE` (UnityCI) pulled by tag; the job resolves a digest at runtime. Logs are sanitized. +- Execution: single pass with immediate per‑test fragment emissions (strict single `<testcase>` per file). A placeholder guard fails fast if any fragment is a bare ID. Staging (`reports/_staging`) is promoted to `reports/` to reduce partial writes. +- Reports: JUnit at `reports/junit-nl-suite.xml`, Markdown at `reports/junit-nl-suite.md`. +- Publishing: JUnit is normalized to `reports/junit-for-actions.xml` and published; artifacts upload all files under `reports/`.MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
35-41: Canonical file missing check is now correct.The inverted logic noted earlier is fixed with the negated File.Exists; condition reads correctly.
🧹 Nitpick comments (4)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (2)
135-163: Broaden legacy root detection to macOS and Windows (avoid Linux‑only bias).Consider adding macOS and Windows legacy locations to reduce false negatives; keep existing checks.
Example:
@@ - string[] legacyRoots = - { - Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), - Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src") - }; + var roots = new System.Collections.Generic.List<string> + { + // Linux + Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), + Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src"), + // macOS + Path.Combine(home, "Library", "Application Support", "UnityMCP", "UnityMcpServer", "src"), + // Windows (best‑guess; verify against past releases) + Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "UnityMCP", "UnityMcpServer", "src"), + Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData), "UnityMCP", "UnityMcpServer", "src"), + };Please verify these match your historical install paths.
1-3: Remove unused using.UnityEngine isn’t referenced here.
-using UnityEngine;TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
275-321: Add a non‑Windows upsert case that preserves an existing env table.You verify exclusion when no env exists; also assert preservation when an env table already exists on non‑Windows.
Example test:
[Test] public void UpsertCodexServerBlock_OnNonWindows_PreservesExistingEnvIfPresent() { MCPServiceLocator.Register<IPlatformService>(new MockPlatformService(isWindows: false)); string existingToml = string.Join("\n", new[] { "[mcp_servers.unityMCP]", "command = \"uv\"", "args = [\"run\", \"--directory\", \"/old/path\", \"server.py\"]", "", "[mcp_servers.unityMCP.env]", "CUSTOM_VAR = \"keep_me\"" }); string result = CodexConfigHelper.UpsertCodexServerBlock(existingToml, "/usr/local/bin/uv", "/srv/src"); using var reader = new StringReader(result); var parsed = TOML.Parse(reader); var env = ((TomlTable)((TomlTable)((TomlTable)parsed["mcp_servers"])["unityMCP"])["env"]); Assert.IsTrue(env.TryGetNode("CUSTOM_VAR", out var custom)); Assert.AreEqual("keep_me", ((TomlString)custom).Value); }MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
54-65: Handle camelCase “mcpServers” when upserting to avoid duplicate sections.TryParse supports both keys, but Upsert only creates/uses snake_case, potentially leaving a stale camelCase block. Migrate or reuse it.
Apply:
@@ - // Ensure mcp_servers table exists - if (!root.TryGetNode("mcp_servers", out var mcpServersNode) || !(mcpServersNode is TomlTable)) - { - root["mcp_servers"] = new TomlTable(); - } - var mcpServers = root["mcp_servers"] as TomlTable; + // Ensure mcp_servers exists; if camelCase exists, migrate it + TomlTable mcpServers = null; + if (root.TryGetNode("mcp_servers", out var mcpServersNode) && mcpServersNode is TomlTable tbl) + { + mcpServers = tbl; + } + else if (root.TryGetNode("mcpServers", out var camelNode) && camelNode is TomlTable camelTbl) + { + mcpServers = camelTbl; + root["mcp_servers"] = mcpServers; + // Remove legacy key to avoid duplicates + root.Remove("mcpServers"); + } + else + { + mcpServers = new TomlTable(); + root["mcp_servers"] = mcpServers; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs(4 hunks)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(2 hunks)docs/CUSTOM_TOOLS.md(4 hunks)docs/README-DEV.md(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/CUSTOM_TOOLS.md
🧰 Additional context used
🧬 Code graph analysis (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (4)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)MCPForUnity/Editor/Services/MCPServiceLocator.cs (3)
MCPServiceLocator(8-76)Reset(56-75)Register(33-51)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)
CodexConfigHelper(15-250)TomlTable(114-135)TomlTable(143-181)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-76)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
IsWindows(25-25)GetSystemRoot(26-26)MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
HasEmbeddedServer(817-820)
🪛 markdownlint-cli2 (0.18.1)
docs/README-DEV.md
210-210: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
131-177: Windows SystemRoot coverage looks solid.Good end‑to‑end assertion of env.SystemRoot in BuildCodexServerBlock, including TOML parse validation.
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
66-79: Env preservation in Upsert is correct.Extracting existing env and passing it through prevents clobbering user keys.
155-173: Windows env merge looks good; keep existing keys intact.Merges an existing env table and only updates SystemRoot; aligns with #311 and fixes #315 behavior.
…g preservation logic
…etection Always overwrite configs
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (1)
143-150: Don’t clobber unityMCP; Upsert must merge and preserve env/user keys.Current implementation replaces the entire unityMCP table and overwrites env, dropping user-defined variables. This conflicts with PR objective #311 and the added tests expecting preservation, and can reintroduce #315 regressions on Windows.
Apply merge semantics in Upsert:
@@ public static string UpsertCodexServerBlock(string existingToml, string uvPath, string serverSrc) - // Parse existing TOML or create new root table - var root = TryParseToml(existingToml) ?? new TomlTable(); - - // Ensure mcp_servers table exists - if (!root.TryGetNode("mcp_servers", out var mcpServersNode) || !(mcpServersNode is TomlTable)) - { - root["mcp_servers"] = new TomlTable(); - } - var mcpServers = root["mcp_servers"] as TomlTable; - - // Create or update unityMCP table - mcpServers["unityMCP"] = CreateUnityMcpTable(uvPath, serverSrc); + // Parse existing TOML or create new root table + var root = TryParseToml(existingToml) ?? new TomlTable(); + + // Reuse existing mcp_servers (or legacy mcpServers) if present + TomlTable mcpServers = null; + if (root.TryGetNode("mcp_servers", out var mcpServersNode) && mcpServersNode is TomlTable tbl) + { + mcpServers = tbl; + } + else if (root.TryGetNode("mcpServers", out var legacyServersNode) && legacyServersNode is TomlTable legacyTbl) + { + // Reuse legacy casing to avoid duplication; write back under mcp_servers + mcpServers = legacyTbl; + root["mcp_servers"] = mcpServers; + } + else + { + mcpServers = new TomlTable(); + root["mcp_servers"] = mcpServers; + } + + // Create or update unityMCP table without clobbering user-defined keys + TomlTable unityMcp; + if (mcpServers.TryGetNode("unityMCP", out var unityNode) && unityNode is TomlTable existingUnity) + { + unityMcp = existingUnity; + } + else + { + unityMcp = new TomlTable(); + mcpServers["unityMCP"] = unityMcp; + } + + // Command/args are authoritative + unityMcp["command"] = new TomlString { Value = uvPath }; + var argsArray = new TomlArray(); + argsArray.Add(new TomlString { Value = "run" }); + argsArray.Add(new TomlString { Value = "--directory" }); + argsArray.Add(new TomlString { Value = serverSrc }); + argsArray.Add(new TomlString { Value = "server.py" }); + unityMcp["args"] = argsArray; + + // Windows-specific env: add/overwrite only SystemRoot, preserve other keys + var platform = MCPServiceLocator.Platform; + if (platform.IsWindows()) + { + TomlTable env; + if (unityMcp.TryGetNode("env", out var envNode) && envNode is TomlTable existingEnv) + env = existingEnv; + else + env = new TomlTable { IsInline = true }; + + var systemRoot = platform.GetSystemRoot(); + if (!string.IsNullOrEmpty(systemRoot)) + env["SystemRoot"] = new TomlString { Value = systemRoot }; + + unityMcp["env"] = env; + }Optionally tighten CreateUnityMcpTable to only add env when Windows (already true) and leave merging to Upsert. The above change should make the new tests pass and honors “append, don’t override.”
Run the EditMode tests for CodexConfigHelperTests to confirm preservation and Windows behavior.
Also applies to: 54-73
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
110-121: Ensure config directory exists before atomic write (JSON and TOML).Guard against missing parent folders when first writing user configs. Leverage the helper in both writers.
Apply:
@@ - McpConfigFileHelper.WriteAtomicFile(configPath, mergedJson); + EnsureConfigDirectoryExists(configPath); + McpConfigFileHelper.WriteAtomicFile(configPath, mergedJson);And similarly in ConfigureCodexClient:
- McpConfigFileHelper.WriteAtomicFile(configPath, updatedToml); + EnsureConfigDirectoryExists(configPath); + McpConfigFileHelper.WriteAtomicFile(configPath, updatedToml);Please confirm whether McpConfigFileHelper.WriteAtomicFile already creates directories; if so, we can skip this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs(3 hunks)MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-76)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (2)
IsWindows(25-25)GetSystemRoot(26-26)MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (4)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)MCPForUnity/Editor/Services/MCPServiceLocator.cs (3)
MCPServiceLocator(8-76)Reset(56-75)Register(33-51)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(15-222)BuildCodexServerBlock(41-52)TomlTable(103-124)TomlTable(131-153)UpsertCodexServerBlock(54-73)
🔇 Additional comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
11-27: Good coverage for Windows/Non‑Windows and env preservation.Tests precisely encode expected behavior (include SystemRoot on Windows; exclude on others; preserve existing env on upsert). They’ll fail with the current Upsert implementation until the merge fix lands.
Re‑run these tests after applying the CodexConfigHelper.UpsertCodexServerBlock merge changes.
Also applies to: 29-35, 131-177, 179-216, 218-273, 275-321, 323-383
… auto-marking as handled
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (2)
82-93: Remove unusedcapturedExvariable.The
capturedExvariable is assigned on line 85 but never used. The error message is already captured in theerrorvariable and stored in EditorPrefs, so this is dead code.Apply this diff to remove the unused variable:
catch (System.Exception ex) { error = ex.Message; - capturedEx = ex; // Store the error for display in the UI, but don't mark as handled // This allows the user to manually rebuild via the "Rebuild Server" button string errorKey = InstallErrorKeyPrefix + version; EditorPrefs.SetString(errorKey, ex.Message ?? "Unknown error");
166-177: Consider removing or renaming this no-op method.The method name
CleanupOldVersionKeyssuggests active cleanup, but the implementation is empty due to Unity's lack of key enumeration support. This is a code smell that could confuse future maintainers.Consider one of these approaches:
- Remove the method and the call on line 71, since it serves no purpose.
- Rename to document intent, e.g.,
// Note: CleanupOldVersionKeys() intentionally not implemented - Unity doesn't support EditorPrefs key enumerationIf you anticipate implementing this in the future, a rename would be better. Otherwise, removal keeps the code cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(3 hunks)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs(1 hunks)MCPForUnity/Editor/Helpers/ServerInstaller.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (2)
PackageLifecycleManager(19-23)ClearLastInstallError(228-240)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (4)
ServerInstaller(14-846)GetServerPath(126-129)EnsureServerInstalled(24-124)HasEmbeddedServer(821-824)
🔇 Additional comments (5)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
491-494: LGTM! Clear error state after successful rebuild.The call to
PackageLifecycleManager.ClearLastInstallError()appropriately clears any previously stored installation error after a successful rebuild. The placement after the success log and before returning true is correct.MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (4)
15-23: LGTM! Standard InitializeOnLoad pattern.The static constructor correctly schedules the installation check after Unity is fully loaded using
EditorApplication.delayCall. The EditorPrefs key constants are well-defined and appropriately prefixed.
25-50: LGTM! Installation trigger logic is sound.The method correctly identifies conditions requiring installation or verification:
- First-time install detection
- Version updates
- Legacy installation presence
- Missing canonical server
The
canonicalMissingcheck correctly uses negation to detect when the file is missing. Error handling provides clear user guidance.
101-134: LGTM! Version resolution with appropriate fallback.The version detection logic correctly prioritizes Unity's package info and falls back to reading the embedded server version file. Error handling returns sensible defaults.
210-240: LGTM! Clean public API for error management.The
GetLastInstallError()andClearLastInstallError()methods provide a clean interface for managing installation error state. The consistent use of version-specific keys and defensive error handling is appropriate. The integration withServerInstaller.RebuildMcpServer()correctly clears errors after successful rebuilds.
…elper into McpConfigurationHelper
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
231-240: Remove unusedArgsEqualmethod.The search confirms
ArgsEqualat line 231 inMCPForUnity/Editor/Helpers/McpConfigurationHelper.csis never called within that file. It's dead code and should be removed.
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
36-44: Resolved: canonicalMissing logic is now correct.Negation added; condition now triggers when server.py is missing. Good fix.
🧹 Nitpick comments (10)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (3)
25-28: Update the method documentation.The comment states the method "only writes when necessary," but according to the AI summary, writes are now unconditional. Please update this documentation to reflect the current behavior.
/// <summary> - /// Writes MCP configuration to the specified path using sophisticated logic - /// that preserves existing configuration and only writes when necessary + /// Writes MCP configuration to the specified path, preserving existing + /// configuration and merging in the unityMCP server block /// </summary>
329-346: Consider adding documentation for the macOS path transformation.This logic transforms
.local/share/UnityMCPpaths toLibrary/Application Supportpaths. While the implementation looks correct, adding a comment explaining why this transformation is necessary (e.g., handling migration from a previous version or cross-platform compatibility) would improve maintainability.try { + // Transform Linux-style .local/share paths to macOS Application Support paths + // for cross-platform compatibility or migration from previous versions if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) && !string.IsNullOrEmpty(serverSrc)) {
348-354: Consider adding documentation for the Windows embedded server logic.The logic that switches from embedded (PackageCache) to installed server when
UseEmbeddedServeris false would benefit from a brief comment explaining the intent.+ // On Windows, if the server is in Unity's PackageCache (embedded) but the user + // preference is to not use the embedded server, switch to the installed server path if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (7)
60-80: Clear stale install error on success.If a previous error exists, it persists in UI despite a successful install. Clear it after a successful EnsureServerInstalled().
Apply:
@@ - // Clean up legacy preference keys - CleanupLegacyPrefs(); + // Clean up legacy preference keys + CleanupLegacyPrefs(); + + // Clear any previous installation error now that installation succeeded + ClearLastInstallError();
93-96: Use Warn for failed installation log.Surface failures more clearly in the Console.
- McpLog.Info($"Server installation failed: {error}. Use Window > MCP For Unity > Rebuild Server to retry.", always: false); + McpLog.Warn($"Server installation failed: {error}. Use Window > MCP For Unity > Rebuild Server to retry.", always: false);
117-132: Centralize embedded version lookup in ServerInstaller to avoid drift.Duplicate path/version logic increases maintenance risk. Delegate to ServerInstaller.
- private static string GetEmbeddedServerVersion() - { - try - { - if (ServerPathResolver.TryFindEmbeddedServerSource(out var embeddedSrc)) - { - var versionPath = Path.Combine(embeddedSrc, "server_version.txt"); - if (File.Exists(versionPath)) - { - return File.ReadAllText(versionPath)?.Trim() ?? "unknown"; - } - } - } - catch { } - return "unknown"; - } + private static string GetEmbeddedServerVersion() + { + try { return ServerInstaller.ReadEmbeddedServerVersion() ?? "unknown"; } + catch { return "unknown"; } + }Add to MCPForUnity/Editor/Helpers/ServerInstaller.cs:
// New public helper to keep version/source logic in one place public static string ReadEmbeddedServerVersion() { if (!TryGetEmbeddedServerSource(out var embeddedSrc)) return null; var versionPath = Path.Combine(embeddedSrc, VersionFileName /* "server_version.txt" */); try { return File.Exists(versionPath) ? File.ReadAllText(versionPath)?.Trim() : null; } catch { return null; } }
134-162: Reuse ServerInstaller legacy detection to cover all OS paths.Current list is Linux-centric and may miss Windows/macOS legacy roots already known to ServerInstaller.
- private static bool LegacyRootsExist() - { - try - { - string home = System.Environment.GetFolderPath( - System.Environment.SpecialFolder.UserProfile - ) ?? string.Empty; - string[] legacyRoots = - { - Path.Combine(home, ".config", "UnityMCP", "UnityMcpServer", "src"), - Path.Combine(home, ".local", "share", "UnityMCP", "UnityMcpServer", "src") - }; - foreach (var root in legacyRoots) - { - try - { - if (File.Exists(Path.Combine(root, "server.py"))) - { - return true; - } - } - catch { } - } - } - catch { } - return false; - } + private static bool LegacyRootsExist() + { + try { return ServerInstaller.LegacyRootsExist(); } catch { return false; } + }Add to MCPForUnity/Editor/Helpers/ServerInstaller.cs:
public static bool LegacyRootsExist() { try { foreach (var legacyRoot in GetLegacyRootsForDetection()) { try { if (File.Exists(Path.Combine(legacyRoot, "server.py"))) return true; } catch { } } } catch { } return false; }
85-91: Avoid “unknown” version key collisions for errors.Store/read errors under a stable “global” key when version cannot be resolved.
@@ - string errorKey = InstallErrorKeyPrefix + version; - EditorPrefs.SetString(errorKey, ex.Message ?? "Unknown error"); + EditorPrefs.SetString(GetErrorKey(version), ex.Message ?? "Unknown error");Add helper near the constants:
@@ private const string InstallErrorKeyPrefix = "MCPForUnity.InstallError:"; // Stores last installation error + + private static string GetErrorKey(string version) + { + return InstallErrorKeyPrefix + (string.IsNullOrEmpty(version) || version == "unknown" ? "global" : version); + }Use it in getters:
@@ public static string GetLastInstallError() - string currentVersion = GetPackageVersion(); - string errorKey = InstallErrorKeyPrefix + currentVersion; - if (EditorPrefs.HasKey(errorKey)) - { - return EditorPrefs.GetString(errorKey, null); - } + var v = GetPackageVersion(); + var key = GetErrorKey(v); + if (EditorPrefs.HasKey(key)) return EditorPrefs.GetString(key, null); @@ public static void ClearLastInstallError() - string currentVersion = GetPackageVersion(); - string errorKey = InstallErrorKeyPrefix + currentVersion; - if (EditorPrefs.HasKey(errorKey)) - { - EditorPrefs.DeleteKey(errorKey); - } + var v = GetPackageVersion(); + var key = GetErrorKey(v); + if (EditorPrefs.HasKey(key)) EditorPrefs.DeleteKey(key);Also applies to: 212-218, 230-236
164-175: Make CleanupOldVersionKeys explicitly a no-op or implement minimal pruning.Unity lacks EditorPrefs enumeration; either document as intentional no-op or maintain a comma-separated list key to prune prior versions.
46-49: Unify menu path text in logs.Standardize to the exact menu label (e.g., “Window > MCP For Unity > Open MCP Window” / “Rebuild Server”) to reduce support friction.
Also applies to: 95-96, 76-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(5 hunks)MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
CodexConfigHelper(15-222)UpsertCodexServerBlock(54-73)
MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (4)
ServerInstaller(14-846)GetServerPath(126-129)EnsureServerInstalled(24-124)HasEmbeddedServer(821-824)
🔇 Additional comments (6)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (5)
109-109: Good refactoring to centralize server resolution and file writing.The extraction of
ResolveServerDirectoryandWriteAtomicFileinto reusable methods improves maintainability. The unconditional write approach ensures configuration stays consistent, which aligns with fixing the TOML configuration issues mentioned in #311.Also applies to: 122-123
275-286: LGTM!The implementation correctly extracts the directory argument with proper null and bounds checking. The logic appropriately returns null when the argument is not found.
288-305: LGTM!The path comparison logic correctly handles platform differences (case-insensitive on Windows, case-sensitive elsewhere) and includes proper exception handling.
359-409: Excellent atomic write implementation.The implementation properly handles atomic file writes with appropriate fallbacks for different platforms and error conditions. The use of UTF8 without BOM is correct for configuration files, and the cleanup logic in the finally block is thorough.
411-443: LGTM with note about side effects.The fallback logic is well-structured and validates each option. Note that this method has side effects (line 422 calls
EnsureServerInstalled), which may install the server if it doesn't exist. This appears intentional given the PR's goal to consolidate package installers, but callers should be aware that "Resolve" may perform installation.MCPForUnity/Editor/Helpers/PackageLifecycleManager.cs (1)
81-91: Good: do not mark installed on failure; persist error per version.This prevents stranding users and allows retries. Nice.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (1)
131-321: Consider testing preservation of existing environment variables in upsert scenarios.The upsert tests verify that existing TOML sections are preserved (lines 248, 304), but they don't test whether existing environment variables within the unityMCP block are preserved when SystemRoot is added or updated on Windows. This scenario could occur if users have manually added custom env variables.
Consider adding a test case like:
[Test] public void UpsertCodexServerBlock_OnWindows_PreservesExistingEnvVariables() { MCPServiceLocator.Register<IPlatformService>(new MockPlatformService(isWindows: true, systemRoot: "C:\\Windows")); string existingToml = string.Join("\n", new[] { "[mcp_servers.unityMCP]", "command = \"uv\"", "args = [\"run\", \"--directory\", \"/path\", \"server.py\"]", "env = { CUSTOM_VAR = \"custom_value\" }" }); string uvPath = "C:\\path\\to\\uv.exe"; string serverSrc = "C:\\path\\to\\server"; string result = CodexConfigHelper.UpsertCodexServerBlock(existingToml, uvPath, serverSrc); TomlTable parsed; using (var reader = new StringReader(result)) { parsed = TOML.Parse(reader); } var unityMcp = parsed["mcp_servers"]["unityMCP"] as TomlTable; var env = unityMcp["env"] as TomlTable; Assert.IsTrue(env.TryGetNode("SystemRoot", out _), "SystemRoot should be added"); Assert.IsTrue(env.TryGetNode("CUSTOM_VAR", out _), "Existing env variables should be preserved"); }MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
345-395: Complex but thorough atomic write implementation.WriteAtomicFile implements a sophisticated write strategy:
- Write to temp file (line 352)
- Use File.Replace for atomic swap when supported (line 355)
- Fall back to manual backup + move on FileNotFoundException (lines 358-362)
- Fall back to manual backup + move on PlatformNotSupportedException (lines 363-376)
- Cleanup temp/backup files (lines 392-393)
The complexity is justified for atomic configuration updates. However, consider adding inline comments to clarify the fallback strategy, especially the PlatformNotSupportedException handler (lines 363-376) which manually implements the replace operation.
Example documentation:
catch (PlatformNotSupportedException) { + // File.Replace not supported on this platform; manually backup and move if (File.Exists(path))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs(5 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (4)
MCPForUnity/Editor/Services/IPlatformService.cs (2)
IsWindows(12-12)GetSystemRoot(18-18)MCPForUnity/Editor/Services/PlatformService.cs (2)
IsWindows(14-17)GetSystemRoot(23-29)MCPForUnity/Editor/Services/MCPServiceLocator.cs (3)
MCPServiceLocator(8-76)Reset(56-75)Register(33-51)MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (5)
CodexConfigHelper(15-222)BuildCodexServerBlock(41-52)TomlTable(103-124)TomlTable(131-153)UpsertCodexServerBlock(54-73)
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (1)
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (2)
CodexConfigHelper(15-222)UpsertCodexServerBlock(54-73)
🔇 Additional comments (9)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/CodexConfigHelperTests.cs (3)
3-5: LGTM! Necessary imports for new test functionality.The new using directives support TOML parsing (Tommy), platform service mocking, and StringReader usage in the new tests.
11-27: LGTM! Clean mock implementation for platform abstraction.The MockPlatformService correctly implements IPlatformService with configurable behavior. The GetSystemRoot method properly returns null for non-Windows platforms (line 26), which aligns with the PlatformService implementation shown in the relevant code snippets.
29-34: LGTM! Essential cleanup to prevent test pollution.Resetting the service locator after each test ensures that mocked platform services don't leak between tests.
MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs (6)
5-5: LGTM! Required for UTF8Encoding without BOM.The System.Text namespace is needed for the UTF8Encoding used in WriteAtomicFile (line 352).
109-109: LGTM! Consistent use of new utility methods.The refactoring successfully consolidates:
- ResolveServerDirectory for server path resolution (lines 109, 184)
- EnsureConfigDirectoryExists for directory preparation (lines 122, 188)
- WriteAtomicFile for safe configuration writes (lines 123, 189)
This improves code reuse and maintainability.
Also applies to: 122-123, 184-184, 188-189
261-272: LGTM! Correct extraction of --directory argument.The loop correctly stops at
args.Length - 1to ensure there's a value to extract after the flag. Returns null when the flag isn't found or is the last element.
274-291: LGTM! Proper cross-platform path comparison.The implementation correctly:
- Normalizes paths using Path.GetFullPath
- Uses case-insensitive comparison on Windows (line 283)
- Uses case-sensitive comparison on other platforms (line 285)
- Handles exceptions gracefully
397-429: LGTM! Good fallback chain for server resolution.The method tries multiple sources in priority order:
- Remembered path from EditorPrefs (lines 401-406)
- Installed server path (lines 408-413)
- Embedded server if flag is set (lines 415-421)
- Final fallback to installed path (line 423)
This ensures the server is found even if one source fails.
317-332: Add an inline comment explaining the macOS path normalization.The path translation from
~/.local/share/UnityMCP/to~/Library/Application Support/UnityMCP/is intentional—Unity/Mono may mapLocalApplicationDatato~/.local/shareon macOS, but the code normalizes it to the standard macOS location. This is documented inServerInstaller.cs(line 143-144) but should be clarified inline here:// Unity/Mono may map LocalApplicationData to ~/.local/share on macOS; // normalize to the standard Application Support directory to mitigate // arg parsing and quoting issues in some MCP clients.
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | ||
| && !string.IsNullOrEmpty(serverSrc) | ||
| && serverSrc.IndexOf(@"\Library\PackageCache\", StringComparison.OrdinalIgnoreCase) >= 0 | ||
| && !EditorPrefs.GetBool("MCPForUnity.UseEmbeddedServer", false)) | ||
| { | ||
| serverSrc = ServerInstaller.GetServerPath(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify embedded server check on Windows.
Lines 334-340 redirect to ServerInstaller.GetServerPath() when the path contains \Library\PackageCache\ and the UseEmbeddedServer preference is false. This logic prevents using the embedded package version on Windows unless explicitly opted in.
Confirm this behavior is correct—should the check also verify that server.py exists at the installer path before redirecting?
🏁 Script executed:
#!/bin/bash
# Search for ServerInstaller.GetServerPath usage and UseEmbeddedServer references
rg -n --type=cs "GetServerPath|UseEmbeddedServer" -A2 -B2Length of output: 12164
Verify that server.py exists before redirecting to ServerInstaller.GetServerPath().
The code at line 339 redirects serverSrc to ServerInstaller.GetServerPath() without confirming that server.py exists at that location. Throughout the codebase, after calling GetServerPath(), the code consistently verifies File.Exists(Path.Combine(path, "server.py")) (see ServerInstaller.cs:100, PackageDetector.cs:24, MCPForUnityEditorWindow.cs:116). At line 410 in the same file, after a similar call, the verification is performed. Add a check before the redirection to prevent attempting to use a missing server.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/McpConfigurationHelper.cs around lines 334-340,
the code switches serverSrc to ServerInstaller.GetServerPath() without verifying
that server.py exists at that target; change the snippet to first call
ServerInstaller.GetServerPath() into a local variable, then check
File.Exists(Path.Combine(localPath, "server.py")) and only assign serverSrc =
localPath when that file exists (leave serverSrc unchanged if it does not).
Ensure you keep the existing OS/EditorPrefs conditions and perform the existence
check before redirecting.
- Added DeleteDirectoryWithRetry helper to handle Windows file locking with retries and readonly attribute clearing - Implemented KillWindowsUvProcesses to safely terminate Python processes in virtual environments using WMIC - Extended TryKillUvForPath to work on Windows, preventing file handle locks during server deletion - Improved error messages to be more descriptive about file locking issues - Replaced direct Directory.Delete calls with
- Added proper socket shutdown sequence using Socket.Shutdown() before closing connections - Enhanced error handling with specific catches for SocketException vs general exceptions - Added debug logging for socket shutdown errors to help diagnose connection issues - Restructured HandleClientAsync to ensure socket cleanup happens in the correct order - Implemented proper socket teardown in both client handling and connection cleanup paths
1 participant
unityMCPfailed to start: request timed out - Codex #315)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests