Add EditorPrefs management window for MCP configuration debugging #491
Conversation
Meant to help with dev and testing, not so much the average user
Reviewer's GuideAdds a new Unity Editor window and service for inspecting and editing MCP-related EditorPrefs, wires it into the MCP menu, and adjusts menu item priorities. Sequence diagram for opening and using the EditorPrefs windowsequenceDiagram
actor UnityDeveloper
participant UnityMenuSystem
participant MCPForUnityMenu
participant EditorPrefsWindow
participant EditorPrefsAPI as EditorPrefs
UnityDeveloper->>UnityMenuSystem: Select Window/MCP For Unity/EditorPrefs
UnityMenuSystem->>MCPForUnityMenu: Invoke ShowEditorPrefsWindow()
MCPForUnityMenu->>EditorPrefsWindow: ShowWindow()
EditorPrefsWindow->>EditorPrefsWindow: GetWindow EditorPrefsWindow
EditorPrefsWindow->>EditorPrefsWindow: CreateGUI()
EditorPrefsWindow->>EditorPrefsWindow: LoadKnownMcpKeys()
loop For each key
EditorPrefsWindow->>EditorPrefsAPI: HasKey(key)
EditorPrefsAPI-->>EditorPrefsWindow: bool
EditorPrefsWindow->>EditorPrefsAPI: GetString/GetInt/GetBool
EditorPrefsAPI-->>EditorPrefsWindow: value
EditorPrefsWindow->>EditorPrefsWindow: CreateEditorPrefItem(key)
EditorPrefsWindow->>EditorPrefsWindow: CreateItemUI(item)
end
UnityDeveloper->>EditorPrefsWindow: Edit value and click Save
EditorPrefsWindow->>EditorPrefsWindow: SavePref(item, newValue, newType)
EditorPrefsWindow->>EditorPrefsAPI: SetString/SetInt/SetFloat/SetBool
EditorPrefsAPI-->>EditorPrefsWindow: persist
EditorPrefsWindow->>EditorPrefsWindow: RefreshPrefs()
Class diagram for new EditorPrefs management windowclassDiagram
class EditorPrefsWindow {
+ScrollView scrollView
+VisualElement prefsContainer
+List~EditorPrefItem~ currentPrefs
+HashSet~string~ knownMcpKeys
+VisualTreeAsset itemTemplate
+static void ShowWindow()
+void CreateGUI()
-void LoadKnownMcpKeys()
-void RefreshPrefs()
-List~string~ GetAllMcpKeys()
-EditorPrefItem CreateEditorPrefItem(string key)
-bool TryGetInt(string key, int value)
-bool TryGetFloat(string key, float value)
-bool TryGetBool(string key, bool value)
-VisualElement CreateItemUI(EditorPrefItem item)
-void SavePref(EditorPrefItem item, string newValue, EditorPrefType newType)
-void SaveValue(string key, string value, EditorPrefType type)
}
class EditorPrefItem {
+string Key
+string Value
+EditorPrefType Type
+bool IsKnown
}
class EditorPrefType {
<<enumeration>>
String
Int
Float
Bool
}
class EditorPrefsWindowService {
-static EditorPrefsWindowService _instance
+static EditorPrefsWindowService Instance
+static void Initialize()
-EditorPrefsWindowService()
+void ShowWindow()
}
class MCPForUnityMenu {
+static void ToggleMCPWindow()
+static void ShowSetupWindow()
+static void ShowEditorPrefsWindow()
}
EditorPrefsWindow "1" o-- "many" EditorPrefItem : displays
EditorPrefItem --> EditorPrefType : uses
EditorPrefsWindowService ..> EditorPrefsWindow : calls_ShowWindow
MCPForUnityMenu ..> EditorPrefsWindow : calls_ShowWindow
MCPForUnityMenu ..> EditorPrefsWindowService : potential_service_usage
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new EditorPrefs management window and singleton service, integrates an EditorPrefs menu item, and reorders existing MCP menu item priorities. The window loads UXML/USS templates, enumerates MCP-related EditorPrefs keys, and supports type-aware viewing and saving of preferences. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- EditorPrefsWindowService is defined as a singleton but never used by the menu or other code paths (and will throw if Instance is accessed without Initialize being called); either wire it into the menu/usage or remove the service entirely to avoid dead/unsafe code.
- In CreateGUI, visualTree is not null-checked after LoadAssetAtPath and will cause a NullReferenceException if the UXML asset cannot be found; add a null check similar to the itemTemplate guard and early-return with a clear error.
- The type inference helpers (TryGetInt/TryGetFloat/TryGetBool) read values via EditorPrefs.GetString, which won’t return values stored as non-string types; consider using the type-specific GetInt/GetFloat/GetBool APIs or a more robust detection strategy to avoid misclassifying existing prefs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- EditorPrefsWindowService is defined as a singleton but never used by the menu or other code paths (and will throw if Instance is accessed without Initialize being called); either wire it into the menu/usage or remove the service entirely to avoid dead/unsafe code.
- In CreateGUI, visualTree is not null-checked after LoadAssetAtPath and will cause a NullReferenceException if the UXML asset cannot be found; add a null check similar to the itemTemplate guard and early-return with a clear error.
- The type inference helpers (TryGetInt/TryGetFloat/TryGetBool) read values via EditorPrefs.GetString, which won’t return values stored as non-string types; consider using the type-specific GetInt/GetFloat/GetBool APIs or a more robust detection strategy to avoid misclassifying existing prefs.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:44-53` </location>
<code_context>
+ string basePath = AssetPathUtility.GetMcpPackageRootPath();
+
+ // Load UXML
+ var visualTree = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(
+ $"{basePath}/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml"
+ );
+
+ // Load item template
+ itemTemplate = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(
+ $"{basePath}/Editor/Windows/EditorPrefs/EditorPrefItem.uxml"
+ );
+
+ if (itemTemplate == null)
+ {
+ Debug.LogError("Failed to load EditorPrefItem.uxml template");
+ return;
+ }
+
+ visualTree.CloneTree(rootVisualElement);
+
+ // Get references
</code_context>
<issue_to_address>
**issue (bug_risk):** Add a null check for the main UXML asset similar to the item template to avoid a potential null reference.
If `EditorPrefsWindow.uxml` fails to load, `visualTree` will be null and `CloneTree(rootVisualElement)` will throw. Mirror the `itemTemplate` null check: log an error and return early when `visualTree` is null.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:161-163` </location>
<code_context>
+ else
+ {
+ // Try to determine type by parsing the stored string value
+ var stringValue = EditorPrefs.GetString(key, "");
+
+ if (TryGetInt(key, out var intValue))
+ {
+ item.Type = EditorPrefType.Int;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Type inference for unknown keys will fail for values stored as non-string EditorPrefs types.
Because the helpers use `EditorPrefs.GetString` again, any value originally stored via `SetInt/SetFloat/SetBool` will likely come back as `""` here, so your inference will fall through and classify it as `String`. That means unknown numeric/bool prefs will be mis-typed and then re-saved as strings. To avoid this, consider first probing with the typed getters (`GetInt/Float/Bool`) and only falling back to `GetString` when you can reliably detect “no value” from their defaults.
Suggested implementation:
```csharp
else if (key == EditorPrefKeys.UnitySocketPort || key == EditorPrefKeys.ValidationLevel ||
key == EditorPrefKeys.LastUpdateCheck || key == EditorPrefKeys.LastStdIoUpgradeVersion)
{
// These are integer values
item.Type = EditorPrefType.Int;
item.Value = EditorPrefs.GetInt(key, 0).ToString();
}
else
{
// Try to determine type by probing typed EditorPrefs accessors first,
// then falling back to string if no other type can be reliably detected.
// Probe int using a sentinel value that should not normally be stored.
const int intSentinel = int.MinValue + 1;
var intValue = EditorPrefs.GetInt(key, intSentinel);
if (intValue != intSentinel)
{
item.Type = EditorPrefType.Int;
item.Value = intValue.ToString();
}
else
{
// Probe float using NaN as a sentinel.
var floatValue = EditorPrefs.GetFloat(key, float.NaN);
if (!float.IsNaN(floatValue))
{
item.Type = EditorPrefType.Float;
item.Value = floatValue.ToString(CultureInfo.InvariantCulture);
}
else
{
// Probe bool by comparing results with different defaults.
var boolWhenDefaultTrue = EditorPrefs.GetBool(key, true);
var boolWhenDefaultFalse = EditorPrefs.GetBool(key, false);
if (boolWhenDefaultTrue != boolWhenDefaultFalse)
{
item.Type = EditorPrefType.Bool;
item.Value = boolWhenDefaultTrue.ToString();
}
else
{
// Fall back to treating the value as a string.
item.Type = EditorPrefType.String;
item.Value = EditorPrefs.GetString(key, string.Empty);
}
}
}
```
1. Add `using System.Globalization;` at the top of `EditorPrefsWindow.cs` if it is not already present, so `CultureInfo.InvariantCulture` compiles.
2. If there are other uses of `TryGetInt` / similar helpers in this file that are part of the same type inference path, consider updating them to follow the same probing pattern to keep behavior consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefItem.uxml (1)
1-21: Clean UI template structure.The UXML template is well-structured with proper class names matching the USS styling. Minor note: the
uienamespace (UnityEditor.UIElements) is declared but not used in this template.🔎 Optional: Remove unused namespace
-<ui:UXML xmlns:ui="UnityEngine.UIElements" xmlns:uie="UnityEditor.UIElements" editor-extension-mode="True"> +<ui:UXML xmlns:ui="UnityEngine.UIElements" editor-extension-mode="True">MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uss (1)
192-214: Consider consolidating duplicate selector definitions.The standalone selectors
.key-section,.value-field,.type-dropdown, and.action-buttons(lines 192-214) appear to duplicate properties already defined in the nested.pref-rowcontext (lines 169-190). If these serve different purposes, consider adding comments clarifying the intent; otherwise, they could be consolidated to reduce duplication.MCPForUnity/Editor/Services/EditorPrefsWindowService.cs (1)
11-53: Service appears unused - consider removing or completing the integration.This singleton service requires
Initialize()to be called beforeInstancecan be accessed, but:
- No initialization hook (e.g.,
[InitializeOnLoad]) is present- The menu item in
MCPForUnityMenu.cscallsEditorPrefsWindow.ShowWindow()directly, bypassing this service entirely- Accessing
Instancewithout prior initialization will throw an exceptionIf the service pattern is needed for other callers (e.g., MCP commands), consider adding an
[InitializeOnLoad]static constructor or lazy initialization. Otherwise, this class may be unnecessary overhead.🔎 Option 1: Add lazy initialization (simpler)
public static EditorPrefsWindowService Instance { get { if (_instance == null) { - throw new Exception("EditorPrefsWindowService not initialized"); + _instance = new EditorPrefsWindowService(); } return _instance; } } - -/// <summary> -/// Initialize the service -/// </summary> -public static void Initialize() -{ - if (_instance == null) - { - _instance = new EditorPrefsWindowService(); - } -}🔎 Option 2: Remove service if not needed
If the service provides no additional value beyond calling
EditorPrefsWindow.ShowWindow(), consider removing it and calling the window directly where needed.MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
122-133: Remove or clarify the hardcoded test key.The hardcoded
"MCPForUnity.TestKey"appears to be placeholder/debug code. Since the window is meant for development and testing, this might be intentional, but it could confuse users or clutter the UI if this key doesn't exist.🔎 Consider removing or documenting
private List<string> GetAllMcpKeys() { // This is a simplified approach - in reality, getting all EditorPrefs is platform-specific // For now, we'll return known MCP keys that might exist var keys = new List<string>(); - - // Add some common MCP keys that might not be in EditorPrefKeys - keys.Add("MCPForUnity.TestKey"); - - // Filter to only those that actually exist - return keys.Where(EditorPrefs.HasKey).ToList(); + return keys; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.csMCPForUnity/Editor/Services/EditorPrefsWindowService.csMCPForUnity/Editor/Services/EditorPrefsWindowService.cs.metaMCPForUnity/Editor/Windows/EditorPrefs.metaMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefItem.uxmlMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefItem.uxml.metaMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs.metaMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.ussMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uss.metaMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxmlMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml.meta
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 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.
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 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/MenuItems/MCPForUnityMenu.cs
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (3)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)GetMcpPackageRootPath(41-80)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (12)
MCPForUnity/Editor/Windows/EditorPrefs.meta (1)
1-8: LGTM!Standard Unity folder meta file with proper structure and unique GUID.
MCPForUnity/Editor/Services/EditorPrefsWindowService.cs.meta (1)
1-11: LGTM!Standard Unity MonoImporter meta file for the service script.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs.meta (1)
1-11: LGTM!Standard Unity MonoImporter meta file for the window script.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml.meta (1)
1-10: LGTM!Standard Unity ScriptedImporter meta file for the UXML asset.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefItem.uxml.meta (1)
1-10: LGTM!Standard Unity ScriptedImporter meta file for the EditorPrefItem UXML asset.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uss.meta (1)
1-11: LGTM!Standard Unity ScriptedImporter meta file for the USS stylesheet asset.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uss (1)
1-228: Well-structured stylesheet with comprehensive styling.The USS file provides thorough styling for the EditorPrefs window with proper hover states and consistent visual design. The hardcoded dark theme colors are appropriate for Unity Editor tooling.
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml (1)
1-30: LGTM - Clean UXML structure for the EditorPrefs window.The layout is well-organized with clear sections for header, add-new-row panel, and prefs container. The styling references and element naming conventions are consistent.
MCPForUnity/Editor/MenuItems/MCPForUnityMenu.cs (1)
30-34: LGTM - Menu integration for the EditorPrefs window.The new menu entry is properly integrated with appropriate priority ordering.
Note: The
EditorPrefsWindowServiceexists but isn't used here. If the service is meant to be the canonical entry point for opening this window, consider usingEditorPrefsWindowService.Instance.ShowWindow()instead for consistency. Otherwise, the service may be unnecessary overhead.MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (3)
188-229: Type detection may not work correctly for non-string EditorPrefs values.The
TryGetInt,TryGetFloat, andTryGetBoolmethods read values viaEditorPrefs.GetString()and attempt to parse them. However, when a value is stored viaEditorPrefs.SetInt()orSetFloat(), Unity stores it natively (not as a string). CallingGetString()on such keys may return an empty string or the value as a string depending on the platform, making type inference unreliable.Since this is a dev/testing tool, this may be acceptable, but it could lead to incorrect type detection for prefs not in the known-keys list.
135-186: Good explicit type handling for known MCP keys.The explicit type mapping for known boolean keys (Lines 140-145) and integer keys (Lines 151-152) ensures these are read correctly. This approach is reliable for the documented MCP keys and gracefully falls back to type inference for unknown keys.
258-305: Solid save implementation with user-friendly error handling.The
SaveValuemethod properly validates type conversions and displays helpful dialog messages when parsing fails. The switch-based dispatch is clean and handles allEditorPrefTypecases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
164-235: Critical: Type inference will corrupt values stored with typed setters.The type inference logic for unknown keys has a fundamental flaw: it uses
EditorPrefs.GetString()to retrieve values that may have been stored withSetInt(),SetFloat(), orSetBool(). Unity's EditorPrefs API doesn't convert typed values to strings when callingGetString()- it returns an empty string instead. This causes:
- Values stored via
SetInt/SetFloat/SetBoolwill be read as empty strings- Type inference will fail and classify them as
Stringtype- If saved, the original typed value will be overwritten with an empty or incorrect string value
Example scenario:
// Original code stores an int EditorPrefs.SetInt("MCPForUnity.CustomValue", 42); // EditorPrefs window loads it EditorPrefs.GetString("MCPForUnity.CustomValue", "") → returns "" // Type inference fails, treats as String with value "" // User saves → overwrites int 42 with string ""As noted in the previous review comment (lines 167-169), you must probe the typed getters (
GetInt,GetFloat,GetBool) with sentinel values first, before falling back toGetString.🔎 Recommended fix using sentinel-based type probing
Replace the entire else block (lines 164-189) and the helper methods (lines 194-235) with this approach:
else { - // Try to determine type by parsing the stored string value - var stringValue = EditorPrefs.GetString(key, ""); - - if (TryGetInt(key, out var intValue)) + // Probe typed getters with sentinels before falling back to string + const int intSentinel = int.MinValue + 1; + var intValue = EditorPrefs.GetInt(key, intSentinel); + if (intValue != intSentinel) { item.Type = EditorPrefType.Int; item.Value = intValue.ToString(); } - else if (TryGetFloat(key, out var floatValue)) + else { - item.Type = EditorPrefType.Float; - item.Value = floatValue.ToString(); + const float floatSentinel = float.NaN; + var floatValue = EditorPrefs.GetFloat(key, floatSentinel); + if (!float.IsNaN(floatValue)) + { + item.Type = EditorPrefType.Float; + item.Value = floatValue.ToString(); + } + else + { + // Probe bool by comparing results with different defaults + var boolTrue = EditorPrefs.GetBool(key, true); + var boolFalse = EditorPrefs.GetBool(key, false); + if (boolTrue != boolFalse) + { + item.Type = EditorPrefType.Bool; + item.Value = boolTrue.ToString(); + } + else + { + // Fall back to string only after all typed probes fail + item.Type = EditorPrefType.String; + item.Value = EditorPrefs.GetString(key, string.Empty); + } + } } - else if (TryGetBool(key, out var boolValue)) - { - item.Type = EditorPrefType.Bool; - item.Value = boolValue.ToString(); - } - else - { - item.Type = EditorPrefType.String; - item.Value = stringValue; - } }After applying this fix, you can safely delete the helper methods
TryGetInt,TryGetFloat, andTryGetBool(lines 194-235) as they are no longer needed.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
128-139: Simplified implementation limits discoverability.The current implementation only checks for a hardcoded test key. While the comment acknowledges this is simplified due to platform-specific challenges, you could improve discoverability by:
- Checking for keys with common MCP prefixes (e.g., "MCPForUnity.", "MCP.")
- Documenting this limitation more explicitly for future maintainers
This is acceptable for a development/debugging tool but may miss dynamically-created preference keys.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (2)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)GetMcpPackageRootPath(41-80)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (7)
32-37: LGTM!Standard Unity EditorWindow initialization pattern with appropriate minimum size constraint.
39-76: LGTM! Null checks properly implemented.The template loading now includes proper null checks for both
visualTreeanditemTemplate, addressing previous review feedback. The error handling will preventNullReferenceExceptionif assets fail to load.
78-90: LGTM!The reflection-based approach correctly identifies constant fields from
EditorPrefKeysusing the appropriate filters (IsLiteral && !IsInitOnly).
92-126: LGTM! Good security consideration.The method correctly aggregates and displays MCP-related EditorPrefs. The exclusion of
CustomerUuid(line 119) is a thoughtful privacy measure to prevent accidental exposure of sensitive identifiers.
237-262: LGTM!UI element creation and data binding are correctly implemented with proper null checking and event wiring.
264-311: LGTM! Robust error handling.The save logic correctly validates type conversions and provides clear user feedback via dialogs when conversion fails. The early return on error prevents invalid values from being saved.
314-334: LGTM!Clean data structures that appropriately model EditorPrefs items and their type system.
…ow.cs" This reverts commit 09bb4e1.
…dow.cs" This reverts commit 6ccbc5e.
…c parsing Replace complex sentinel-based type detection with a dictionary of known pref types and simple TryParse fallback for unknown keys. Remove null handling and HasKey checks for known keys since they're defined in EditorPrefKeys.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (3)
171-182: Remove hardcoded test key.Line 178 adds a hardcoded
"MCPForUnity.TestKey"which appears to be debug code. While it's filtered byHasKey, this test key shouldn't be in production code.🔎 Proposed fix
private List<string> GetAllMcpKeys() { // This is a simplified approach - in reality, getting all EditorPrefs is platform-specific // For now, we'll return known MCP keys that might exist var keys = new List<string>(); - // Add some common MCP keys that might not be in EditorPrefKeys - keys.Add("MCPForUnity.TestKey"); - // Filter to only those that actually exist return keys.Where(EditorPrefs.HasKey).ToList(); }
184-247: Type detection for unknown keys may be unreliable.Lines 221-243 use
EditorPrefs.GetString()followed byTryParseto infer types for unknown keys. This approach will misidentify types if a value was stored using typed setters (SetInt/SetFloat/SetBool) sinceGetStringreturns""for those, causingTryParseto fail or return incorrect results.However, since
knownPrefTypesnow covers all documented MCP keys, this only affects truly undocumented keys during debugging sessions.Additionally, float conversions (lines 204, 229, 232) should use
CultureInfo.InvariantCultureto avoid locale-dependent formatting issues.🔎 Proposed improvements
Option 1: Document the limitation (minimal change)
Add a comment explaining the limitation:
else { // Only try to detect type for unknown keys that actually exist if (!EditorPrefs.HasKey(key)) { // Key doesn't exist and we don't know its type, skip it return null; } + // Note: Type detection using GetString + TryParse may misidentify + // values stored via SetInt/SetFloat/SetBool since GetString returns "" + // for those. This only affects undocumented/unknown keys. // Unknown pref - try to detect type var stringValue = EditorPrefs.GetString(key, "");Option 2: Use culture-invariant float parsing
case EditorPrefType.Float: item.Type = EditorPrefType.Float; - item.Value = EditorPrefs.GetFloat(key, 0f).ToString(); + item.Value = EditorPrefs.GetFloat(key, 0f).ToString(CultureInfo.InvariantCulture); break;And in the unknown key detection:
- else if (float.TryParse(stringValue, out var floatValue)) + else if (float.TryParse(stringValue, NumberStyles.Float, CultureInfo.InvariantCulture, out var floatValue)) { item.Type = EditorPrefType.Float; - item.Value = floatValue.ToString(); + item.Value = floatValue.ToString(CultureInfo.InvariantCulture); }
282-323: Use culture-invariant float parsing.Line 301 uses
float.TryParsewithout specifying culture, which can cause issues in locales that use comma as decimal separator (e.g., "3.14" fails in German locale). UseCultureInfo.InvariantCulturefor consistent parsing.🔎 Proposed fix
case EditorPrefType.Float: - if (float.TryParse(value, out var floatValue)) + if (float.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out var floatValue)) { EditorPrefs.SetFloat(key, floatValue); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (5)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-48)MCPForUnity/Editor/Tools/ManageScript.cs (1)
ValidationLevel(1946-1950)MCPForUnity/Editor/Services/EditorPrefsWindowService.cs (1)
ShowWindow(49-52)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)GetMcpPackageRootPath(41-80)MCPForUnity/Editor/Helpers/McpLog.cs (1)
Debug(31-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (3)
28-63: Well-structured type mapping dictionary.The
knownPrefTypesdictionary effectively addresses the type inference concerns from previous reviews by explicitly mapping all known MCP EditorPrefs keys to their correct types. This approach is much more reliable than runtime type detection.
117-129: Effective use of reflection to discover known keys.The reflection logic correctly identifies
constfields usingIsLiteral && !IsInitOnly, ensuring all MCP keys defined inEditorPrefKeysare automatically discovered.
326-346: Clean data model definitions.The
EditorPrefItemandEditorPrefTypestructures are appropriately simple for representing EditorPrefs entries with type information.
| public void CreateGUI() | ||
| { | ||
| string basePath = AssetPathUtility.GetMcpPackageRootPath(); | ||
|
|
||
| // Load UXML | ||
| var visualTree = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>( | ||
| $"{basePath}/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml" | ||
| ); | ||
|
|
||
| if (visualTree == null) | ||
| { | ||
| Debug.LogError("Failed to load EditorPrefsWindow.uxml template"); | ||
| return; | ||
| } | ||
|
|
||
| // Load item template | ||
| itemTemplate = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>( | ||
| $"{basePath}/Editor/Windows/EditorPrefs/EditorPrefItem.uxml" | ||
| ); | ||
|
|
||
| if (itemTemplate == null) | ||
| { | ||
| Debug.LogError("Failed to load EditorPrefItem.uxml template"); | ||
| return; | ||
| } | ||
|
|
||
| visualTree.CloneTree(rootVisualElement); | ||
|
|
||
| // Get references | ||
| scrollView = rootVisualElement.Q<ScrollView>("scroll-view"); | ||
| prefsContainer = rootVisualElement.Q<VisualElement>("prefs-container"); | ||
|
|
||
| // Load known MCP keys | ||
| LoadKnownMcpKeys(); | ||
|
|
||
| // Load initial data | ||
| RefreshPrefs(); | ||
| } |
There was a problem hiding this comment.
Add null checks for basePath and queried UI elements.
Three potential null reference issues:
- Line 80:
AssetPathUtility.GetMcpPackageRootPath()can return null (per snippet 2), but there's no null check before using it in path construction. - Lines 107-108:
scrollViewandprefsContainercould be null if the UXML structure doesn't match expectations, which would cause exceptions duringRefreshPrefs().
🔎 Proposed fix
public void CreateGUI()
{
string basePath = AssetPathUtility.GetMcpPackageRootPath();
+
+ if (string.IsNullOrEmpty(basePath))
+ {
+ Debug.LogError("Failed to get MCP package root path");
+ return;
+ }
// Load UXML
var visualTree = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(
$"{basePath}/Editor/Windows/EditorPrefs/EditorPrefsWindow.uxml"
);
if (visualTree == null)
{
Debug.LogError("Failed to load EditorPrefsWindow.uxml template");
return;
}
// Load item template
itemTemplate = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(
$"{basePath}/Editor/Windows/EditorPrefs/EditorPrefItem.uxml"
);
if (itemTemplate == null)
{
Debug.LogError("Failed to load EditorPrefItem.uxml template");
return;
}
visualTree.CloneTree(rootVisualElement);
// Get references
scrollView = rootVisualElement.Q<ScrollView>("scroll-view");
prefsContainer = rootVisualElement.Q<VisualElement>("prefs-container");
+
+ if (scrollView == null || prefsContainer == null)
+ {
+ Debug.LogError("Failed to find required UI elements in EditorPrefsWindow.uxml");
+ return;
+ }
// Load known MCP keys
LoadKnownMcpKeys();
// Load initial data
RefreshPrefs();
}🤖 Prompt for AI Agents
In MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs around lines 78
to 115, add null checks for basePath and for the queried UI elements: if
AssetPathUtility.GetMcpPackageRootPath() returns null, log an error and return
before building paths; after loading and cloning the visualTree, verify
scrollView and prefsContainer are not null (log an error and return if either is
null) before calling LoadKnownMcpKeys() / RefreshPrefs() so subsequent calls do
not throw NullReferenceExceptions.
There was a problem hiding this comment.
I'd classify this as minor. If these don't exist, a dev will pick it up in testing. We'll always test UI logic when making changes
There was a problem hiding this comment.
@msanatan, understood! I appreciate you sharing your team's testing approach. Since you have a robust testing process for UI logic and these issues will be caught during development, I agree the risk is lower than initially flagged. Thanks for the clarification!
✏️ Learnings added
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In the MCPForUnity project, the team prefers to rely on their testing process to catch UI initialization issues rather than adding defensive null checks for UI elements in editor windows.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@dsarno @Scriptwonder just an FYI, it's not a major update, but this is something that helped me debugging some issues and features. I don't expect regular users to use it. I had a more rudimentary tool to see editor prefs I cared about, this should be a bit more polished and can maybe help you when debugging. 
|
|
Love it. Nice work @msanatan. Kind of surprising Unity doesn't have this built in! Are there any in there that we're not using anymore? |
|
Thanks @dsarno! I've checked through the code, I believe all have a valid use still! |
2 participants
Meant to help with dev and testing, not so much the average user
Summary by Sourcery
Add an EditorPrefs management window to aid MCP-for-Unity development and expose it via the Unity editor menu.
New Features:
Enhancements:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.