fix: Changed flag management to EditorPrefs #408
Conversation
WalkthroughReplaces a per-session static flag with SessionState persistence for setup-window gating, adds public helpers to query/close MCP editor windows ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
MCPForUnity/Editor/Setup/SetupWindowService.cs (3)
57-61: Good readability improvement.The multi-line formatting makes the complex conditional easier to parse. Each boolean sub-expression is now clearly visible on its own line.
63-66: Consistent log formatting.The multi-line format improves readability for the lengthy log message and aligns with the formatting applied to the other log call later in the method.
80-83: Consistent log formatting.This maintains the same multi-line format applied to the earlier log call, keeping the code style consistent throughout the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Setup/SetupWindowService.cs(4 hunks)
🔇 Additional comments (2)
MCPForUnity/Editor/Setup/SetupWindowService.cs (2)
21-25: LGTM! Clear documentation of SessionState usage.The constant naming and inline comments clearly explain why SessionState is used over a static field. This documentation will help future maintainers understand the persistence behavior across domain reloads.
42-47: Based on my web search verification, I can now provide the rewritten review comment. The technical claims in the original review have been confirmed:SessionState API verification complete — implementation is correct.
SessionState (UnityEditor.SessionState) is the appropriate API for this use case. It survives domain reloads and Play Mode transitions within the same Editor session, making it ideal for "run once per session" logic. The implementation correctly:
- Checks if already executed via
SessionState.GetBoolwith a safe default offalse- Returns early on subsequent checks
- Marks as executed via
SessionState.SetBoolbefore running setup logic- Prevents repeated checks even if early returns occur from exceptions or conditional logic
The pattern matches Unity's recommended approach for Editor session-scoped state. No issues found.
…etter readability and maintainability
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
30-55: Open-window helpers look solid; consider unified loggingThe static
OpenWindowsset withHasAnyOpenWindow/CloseAllOpenWindowsis a clean way to coordinate with external callers, and copying to an array before closing avoids collection-modification issues.For consistency with the rest of the plugin’s logging (and to keep the MCP prefix), consider routing the warning through
McpLoginstead ofDebug.LogWarning:- catch (Exception ex) - { - Debug.LogWarning($"Error closing MCP window: {ex.Message}"); - } + catch (Exception ex) + { + McpLog.Warn($"Error closing MCP window: {ex.Message}"); + }Optionally, if
RequestHealthVerificationever grows logic that might close windows, you may want to mirror the “copy to array” pattern there as well.
111-115: Cross-section event wiring is safe; watch for repeatedCreateGUIcallsThe new callbacks from
McpSettingsSectionandMcpConnectionSectionintoclientConfigSection/connectionSectionare wired safely with null-conditional access, so missing sections won’t throw.One thing to be aware of: if Unity ever calls
CreateGUI()more than once on the same window instance (e.g., under certain domain reload patterns), these+=subscriptions could accumulate. If that’s a concern for your target Unity versions, you could either unsubscribe inOnDisableor guardCreateGUIso wiring happens only once per window.Also applies to: 126-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/Windows/MCPForUnityEditorWindow.cs
📚 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/Windows/MCPForUnityEditorWindow.cs
🧬 Code graph analysis (1)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (2)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
Debug(31-35)McpLog(7-52)Error(48-51)MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
UpdateManualConfiguration(155-178)
🔇 Additional comments (3)
MCPForUnity/Editor/Windows/MCPForUnityEditorWindow.cs (3)
7-11: Namespace imports match usagesThe added using directives line up with the types used in this file (sections, Editor APIs, UI Toolkit) and keep the class self-contained; no issues here.
68-70: UXML load error logging is clear and consistentReformatting the
McpLog.Errorcall into a multi-line statement with the full path interpolated improves readability and keeps error reporting consistent with the rest of the editor tooling.
199-204: Improved null-safety in scheduled health checksThe guard clause
if (this == null || connectionSection == null) { return; }correctly prevents a NullReferenceException. In C#, awaiting a null Task always throws NullReferenceException at runtime, so the null-check beforeawait connectionSection.VerifyBridgeConnectionAsync();is essential. This handles both the destroyed-window case (this == null) and the uninitialized connection section case (connectionSection == null), making the delayed health check safe.If you later introduce code that can swap out
connectionSection, snapshotting it into a local before the null-check would provide extra safety, but this fix is solid as-is.
…ed CreateGUI calls
|
@Hashibutogarasu Thanks for your contribution, this looks good and is a nice little cleanup and ux improvement. Merging now. One small note: your title says “Changed flag management to EditorPrefs” but the final implementation uses SessionState (which is a good way to do it) |
2 participants
This pull request updates the logic for showing the setup window in the Unity Editor to ensure it only appears once per editor session, even across domain reloads. The main improvement is switching from a static boolean flag to Unity's
SessionState, which persists across assembly reloads and playmode transitions. Additionally, logging statements have been refactored for clarity and consistency.Setup window session management:
_hasCheckedThisSessionflag with a persistentSessionStatekey (SessionCheckedKey) to track if the setup check has already run in the current editor session. This prevents the setup window from reappearing after script recompiles or playmode toggles.CheckSetupNeeded()to useSessionState.GetBoolandSessionState.SetBoolfor session tracking, ensuring the check only runs once per session.Logging improvements:
Minor cleanup:
Summary by CodeRabbit
Improvements
New Features
✏️ Tip: You can customize this high-level summary in your review settings.