Merged
Conversation
Add a number of UI management actions and helpers to the Unity editor tool and server interface. Implemented new editor actions: update_panel_settings, render_ui (editor + play-mode screenshot capture with RT lifecycle), link_stylesheet (insert Style src into UXML), delete (asset deletion), list (paginated UI asset listing), detach_ui_document, and modify_visual_element (change text, classes, inline styles, visibility, tooltip). Refactored PanelSettings creation to accept a generic 'settings' dict and introduced ApplyPanelSettingsProperties with parsing helpers for enums, colors, numeric and composite fields. Persist RenderTextures per PanelSettings for render_ui and added play-mode coroutine capture. Server-side tool metadata and routing updated to expose new actions and parameters; mutation routing updated accordingly. Added integration tests for render_ui and link_stylesheet and Unity edit-mode tests for delete. Misc: added using imports and various helper functions for path/file handling and inline style application.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant Unity as UnityEditor
participant Assets as AssetDB
Client->>Server: manage_ui(action, params)
Server->>Unity: send_with_unity_instance / send_mutation
Unity->>Assets: read/write UXML/USS, PanelSettings, stylesheets
Unity->>Unity: attach/detach UIDocument, modify VisualElements, build visual tree
Unity->>Unity: render UI -> produce image/RenderTexture
Unity-->>Server: result (dict with base64 contents/images if applicable)
Server-->>Client: normalized response (decoded contents / metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
Server/src/services/tools/manage_ui.py (2)
245-250: Inconsistent type handling betweenenabledandvisibleparameters.
enabledis passed as a boolean whilevisibleis converted to a string ("true"/"false"). While this matches the C# implementation's expectations, it creates an inconsistent API surface. Consider documenting this behavior in the parameter annotation or normalizing both to the same type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_ui.py` around lines 245 - 250, The code treats enabled as a boolean but converts visible to a lowercase string, creating an inconsistent API; update the handling in the block that populates params_dict so both enabled and visible are normalized the same way (e.g., set params_dict["enabled"] = str(enabled).lower() when enabled is not None and keep params_dict["visible"] = str(visible).lower()), or alternatively add explicit type annotations/comments on the function parameters (enabled, visible) to document that the C# backend expects "true"/"false" strings; adjust the code around params_dict, enabled, visible, and tooltip accordingly to keep types consistent.
279-282: Silent exception swallowing obscures decoding failures.The
try-except-passpattern here silently discards base64 decoding errors, making debugging difficult if malformed data is received. Per static analysis hints (S110, BLE001).♻️ Proposed fix to log decoding errors
+import logging + +logger = logging.getLogger(__name__) + # In the decode block: try: decoded = base64.b64decode( data["encodedContents"]).decode("utf-8") data["contents"] = decoded del data["encodedContents"] del data["contentsEncoded"] - except Exception: - pass + except Exception as e: + logger.debug("Failed to decode base64 contents: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_ui.py` around lines 279 - 282, The code is silently swallowing decoding errors around deleting data["encodedContents"] and data["contentsEncoded"]; replace the bare except/pass with an explicit except Exception as e that logs the failure (e.g., logger.error or logger.exception) including the exception and context (the offending keys or a snippet of data) so decoding problems are visible, then continue to preserve existing behavior; specifically modify the try/except block that touches data["encodedContents"] and data["contentsEncoded"] to log the error (and stacktrace if available) instead of passing.MCPForUnity/Editor/Tools/ManageUI.cs (1)
956-959:Canvas.ForceUpdateCanvases()is ineffective for UI Toolkit.UI Toolkit doesn't use the Canvas system—it has its own rendering pipeline via PanelSettings and UIDocument. This call is a no-op here. The
MarkDirtyRepaint()andRepaintAllViews()calls above are the appropriate methods for UI Toolkit.♻️ Consider removing the ineffective call
// Mark dirty and force editor repaint so the panel renders into the RT uiDoc.rootVisualElement?.MarkDirtyRepaint(); EditorUtility.SetDirty(panelSettings); UnityEditorInternal.InternalEditorUtility.RepaintAllViews(); - - // Force a synchronous layout + repaint pass - Canvas.ForceUpdateCanvases();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageUI.cs` around lines 956 - 959, Canvas.ForceUpdateCanvases() is a no-op for UI Toolkit; remove the Canvas.ForceUpdateCanvases() call and rely on the existing UI Toolkit refresh calls (MarkDirtyRepaint() / RepaintAllViews()) instead, or replace that line with a short comment noting UI Toolkit uses PanelSettings/UIDocument rendering so Canvas.ForceUpdateCanvases() is unnecessary.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs (1)
406-456: Consider adding tests forlink_stylesheetaction.The
link_stylesheetaction is implemented in ManageUI.cs but not covered by tests here. Since it modifies UXML file content by injecting stylesheet references, it would benefit from test coverage.Would you like me to generate test cases for the
link_stylesheetaction?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs` around lines 406 - 456, Add unit tests in ManageUITests.cs covering the ManageUI.HandleCommand "link_stylesheet" action: create a temp UXML file (use TempRoot and ToJObject as in existing tests), call ManageUI.HandleCommand with ["action"]="link_stylesheet", ["path"]=uxmlPath, ["stylesheet"]="path/to/style.uss", assert success and that the saved UXML file content now contains the injected stylesheet reference; add tests for idempotency (running link twice doesn't duplicate the reference), for missing stylesheet path returning an error, and for invalid target extension returning the same ".uxml or .uss" error behavior; use the same patterns and helpers (ManageUI.HandleCommand, ToJObject, TempRoot) as the surrounding tests.unity-mcp-skill/references/tools-reference.md (1)
589-657: Mirror docs should include the fullmanage_uiaction set.Please mirror the same extended action coverage here (
render_ui,link_stylesheet,delete,list,detach_ui_document,modify_visual_element,update_panel_settings) so both tool-reference trees stay complete and consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/references/tools-reference.md` around lines 589 - 657, The manage_ui doc only lists a subset of actions; extend the examples and descriptions to include the full action set: render_ui, link_stylesheet, delete, list, detach_ui_document, modify_visual_element, and update_panel_settings so the tool-reference mirrors the other tree; for each action add a short Python example call to manage_ui(action="...") showing required params and expected return shape, plus a one-line description of behavior and any optional args, referencing the same function name manage_ui and action strings to locate where to insert the new examples..claude/skills/unity-mcp-skill/references/tools-reference.md (1)
589-657: Expandmanage_uiaction coverage in the reference section.This section currently documents core actions, but PR scope includes additional actions (
render_ui,link_stylesheet,delete,list,detach_ui_document,modify_visual_element,update_panel_settings). Since this file is the canonical tool reference, add at least brief examples/notes for those actions too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/unity-mcp-skill/references/tools-reference.md around lines 589 - 657, The tools reference is missing docs/examples for additional manage_ui actions added in the PR; update the UI Tools section to include brief examples/notes for manage_ui with actions render_ui, link_stylesheet, delete, list, detach_ui_document, modify_visual_element, and update_panel_settings so users know their parameters and expected return shapes. Locate the manage_ui examples block and append concise examples for each new action showing required keys (e.g., action="render_ui", target="UICanvas"; action="link_stylesheet", target="UICanvas", source_asset="Assets/UI/Styles.uss"; action="delete", path="Assets/UI/Old.uxml"; action="list", directory="Assets/UI"; action="detach_ui_document", target="UICanvas"; action="modify_visual_element", target="UICanvas", selector={"name":"Title"}, changes={"text":"New"}; action="update_panel_settings", path="Assets/UI/Panel.asset", scale_mode="ScaleWithScreenSize") and a one-line note about typical return values (success/data or error) for each to match the style of the existing create/read/update examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 716-733: The static play-mode capture state (s_pendingCaptureTex,
s_pendingCaptureDone, s_pendingCaptureStarted) can be corrupted by concurrent
render_ui calls; update the flow to track a unique capture token per request and
only satisfy the matching request: have render_ui generate a GUID/ID, pass it
into the MCP_ScreenCapturer instance (store it as a field on the created
GameObject/MonoBehaviour), and when Start() captures the texture set the static
state or invoke a completion callback only if the captured ID matches the
current request ID (or better, replace the shared statics with a Task/Action
stored on the MCP_ScreenCapturer instance and have Start() call that
instance-specific completion). Ensure render_ui checks/sets
s_pendingCaptureStarted atomically (or rejects new requests) and document the
single-in-flight behavior if you choose to reject concurrent captures.
- Around line 725-732: Start() calls ScreenCapture.CaptureScreenshotAsTexture()
without verifying the ScreenCapture API is available which can throw if the
module is disabled; modify Start() (the coroutine method) to first check for the
ScreenCapture availability (e.g., via Application.HasProLicense or
UnityEngine.ScreenCapture existence check / conditional compilation) or wrap the
call in a try/catch, and on failure set ManageUI.s_pendingCaptureDone = false,
ManageUI.s_pendingCaptureStarted = false (or log the error) and skip assigning
ManageUI.s_pendingCaptureTex, then Destroy(gameObject) to avoid throwing;
reference Start(), ManageUI.s_pendingCaptureTex, ManageUI.s_pendingCaptureDone,
and ManageUI.s_pendingCaptureStarted when making the change.
In `@Server/tests/integration/test_manage_ui.py`:
- Around line 188-195: The test assigns resp =
run_async(manage_ui_mod.manage_ui(...)) but never uses resp, causing an unused
variable warning; remove the unused assignment or use the result. Update the
test in test_manage_ui.py to either drop the resp variable and call
run_async(manage_ui_mod.manage_ui(...)) for its side effects, or assert
something about the returned value from manage_ui (refer to the manage_ui
function) if the response should be validated, ensuring no unused local named
resp remains.
---
Nitpick comments:
In @.claude/skills/unity-mcp-skill/references/tools-reference.md:
- Around line 589-657: The tools reference is missing docs/examples for
additional manage_ui actions added in the PR; update the UI Tools section to
include brief examples/notes for manage_ui with actions render_ui,
link_stylesheet, delete, list, detach_ui_document, modify_visual_element, and
update_panel_settings so users know their parameters and expected return shapes.
Locate the manage_ui examples block and append concise examples for each new
action showing required keys (e.g., action="render_ui", target="UICanvas";
action="link_stylesheet", target="UICanvas",
source_asset="Assets/UI/Styles.uss"; action="delete", path="Assets/UI/Old.uxml";
action="list", directory="Assets/UI"; action="detach_ui_document",
target="UICanvas"; action="modify_visual_element", target="UICanvas",
selector={"name":"Title"}, changes={"text":"New"};
action="update_panel_settings", path="Assets/UI/Panel.asset",
scale_mode="ScaleWithScreenSize") and a one-line note about typical return
values (success/data or error) for each to match the style of the existing
create/read/update examples.
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 956-959: Canvas.ForceUpdateCanvases() is a no-op for UI Toolkit;
remove the Canvas.ForceUpdateCanvases() call and rely on the existing UI Toolkit
refresh calls (MarkDirtyRepaint() / RepaintAllViews()) instead, or replace that
line with a short comment noting UI Toolkit uses PanelSettings/UIDocument
rendering so Canvas.ForceUpdateCanvases() is unnecessary.
In `@Server/src/services/tools/manage_ui.py`:
- Around line 245-250: The code treats enabled as a boolean but converts visible
to a lowercase string, creating an inconsistent API; update the handling in the
block that populates params_dict so both enabled and visible are normalized the
same way (e.g., set params_dict["enabled"] = str(enabled).lower() when enabled
is not None and keep params_dict["visible"] = str(visible).lower()), or
alternatively add explicit type annotations/comments on the function parameters
(enabled, visible) to document that the C# backend expects "true"/"false"
strings; adjust the code around params_dict, enabled, visible, and tooltip
accordingly to keep types consistent.
- Around line 279-282: The code is silently swallowing decoding errors around
deleting data["encodedContents"] and data["contentsEncoded"]; replace the bare
except/pass with an explicit except Exception as e that logs the failure (e.g.,
logger.error or logger.exception) including the exception and context (the
offending keys or a snippet of data) so decoding problems are visible, then
continue to preserve existing behavior; specifically modify the try/except block
that touches data["encodedContents"] and data["contentsEncoded"] to log the
error (and stacktrace if available) instead of passing.
In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs`:
- Around line 406-456: Add unit tests in ManageUITests.cs covering the
ManageUI.HandleCommand "link_stylesheet" action: create a temp UXML file (use
TempRoot and ToJObject as in existing tests), call ManageUI.HandleCommand with
["action"]="link_stylesheet", ["path"]=uxmlPath,
["stylesheet"]="path/to/style.uss", assert success and that the saved UXML file
content now contains the injected stylesheet reference; add tests for
idempotency (running link twice doesn't duplicate the reference), for missing
stylesheet path returning an error, and for invalid target extension returning
the same ".uxml or .uss" error behavior; use the same patterns and helpers
(ManageUI.HandleCommand, ToJObject, TempRoot) as the surrounding tests.
In `@unity-mcp-skill/references/tools-reference.md`:
- Around line 589-657: The manage_ui doc only lists a subset of actions; extend
the examples and descriptions to include the full action set: render_ui,
link_stylesheet, delete, list, detach_ui_document, modify_visual_element, and
update_panel_settings so the tool-reference mirrors the other tree; for each
action add a short Python example call to manage_ui(action="...") showing
required params and expected return shape, plus a one-line description of
behavior and any optional args, referencing the same function name manage_ui and
action strings to locate where to insert the new examples.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.claude/skills/unity-mcp-skill/SKILL.md.claude/skills/unity-mcp-skill/references/tools-reference.md.claude/skills/unity-mcp-skill/references/workflows.mdMCPForUnity/Editor/Resources/Project/ProjectInfo.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Tools/ManageUI.cs.metaREADME.mdServer/src/services/tools/manage_ui.pyServer/tests/integration/test_manage_ui.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs.metadocs/i18n/README-zh.mdmanifest.jsonunity-mcp-skill/SKILL.mdunity-mcp-skill/references/tools-reference.mdunity-mcp-skill/references/workflows.md
Contributor
There was a problem hiding this comment.
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MCPForUnity/Editor/Tools/ManageUI.cs
Outdated
Comment on lines
+728
to
+731
| ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture(); | ||
| ManageUI.s_pendingCaptureDone = true; | ||
| ManageUI.s_pendingCaptureStarted = false; | ||
| Destroy(gameObject); |
There was a problem hiding this comment.
The render_ui action uses ScreenCapture.CaptureScreenshotAsTexture() in play mode without checking if the ScreenCapture module is available. This could cause a runtime error if the module is disabled in Unity's Package Manager. Consider adding a check using ScreenshotUtility.IsScreenCaptureModuleAvailable before calling ScreenCapture API, or wrapping it in a try-catch with a helpful error message directing users to enable the module.
Suggested change
| ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture(); | |
| ManageUI.s_pendingCaptureDone = true; | |
| ManageUI.s_pendingCaptureStarted = false; | |
| Destroy(gameObject); | |
| try | |
| { | |
| ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture(); | |
| } | |
| catch (Exception ex) | |
| { | |
| Debug.LogError( | |
| "Failed to capture screenshot texture. The ScreenCapture module may be disabled in the Unity Package Manager. " + | |
| "Please enable the ScreenCapture module to use UI rendering features.\n" + ex); | |
| ManageUI.s_pendingCaptureTex = null; | |
| } | |
| finally | |
| { | |
| ManageUI.s_pendingCaptureDone = true; | |
| ManageUI.s_pendingCaptureStarted = false; | |
| Destroy(gameObject); | |
| } |
Comment on lines
+1111
to
+1112
| string styleTag = $"\n <Style src=\"project://database/{stylesheetPath}\" />"; | ||
| content = content.Insert(insertIdx, styleTag); |
There was a problem hiding this comment.
The link_stylesheet action inserts the stylesheet path directly into UXML content without XML escaping. While the path is validated by AssetPathUtility.SanitizeAssetPath (which rejects special characters including angle brackets), it would be safer to explicitly XML-escape the path when inserting it. Consider using System.Security.SecurityElement.Escape(stylesheetPath) or a similar method to prevent any potential injection if the validation logic changes in the future.
MCPForUnity/Editor/Tools/ManageUI.cs
Outdated
Comment on lines
+715
to
+950
| private static readonly Dictionary<int, RenderTexture> s_panelRTs = new(); | ||
|
|
||
| // Play-mode coroutine capture state | ||
| private static Texture2D s_pendingCaptureTex; | ||
| private static bool s_pendingCaptureDone; | ||
| private static bool s_pendingCaptureStarted; | ||
|
|
||
| // MonoBehaviour that captures a screenshot at end-of-frame in play mode. | ||
| private sealed class MCP_ScreenCapturer : MonoBehaviour | ||
| { | ||
| private System.Collections.IEnumerator Start() | ||
| { | ||
| yield return new WaitForEndOfFrame(); | ||
| ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture(); | ||
| ManageUI.s_pendingCaptureDone = true; | ||
| ManageUI.s_pendingCaptureStarted = false; | ||
| Destroy(gameObject); | ||
| } | ||
| } | ||
|
|
||
| private static object RenderUI(JObject @params) | ||
| { | ||
| var p = new ToolParams(@params); | ||
|
|
||
| string target = p.Get("target"); | ||
| string uxmlPath = p.Get("path"); | ||
| int width = p.GetInt("width") ?? 1920; | ||
| int height = p.GetInt("height") ?? 1080; | ||
| bool includeImage = p.GetBool("include_image") || p.GetBool("includeImage"); | ||
| int maxResolution = p.GetInt("max_resolution") ?? p.GetInt("maxResolution") ?? 640; | ||
| string fileName = p.Get("file_name") ?? p.Get("fileName"); | ||
|
|
||
| if (string.IsNullOrEmpty(target) && string.IsNullOrEmpty(uxmlPath)) | ||
| { | ||
| return new ErrorResponse("Either 'target' (GameObject with UIDocument) or 'path' (UXML asset path) is required."); | ||
| } | ||
|
|
||
| // ── Play-mode capture via ScreenCapture coroutine ────────────────────── | ||
| // PanelSettings.targetTexture is read in the same frame it is assigned, | ||
| // so the RT is always blank in a synchronous tool call. In play mode we | ||
| // dispatch a WaitForEndOfFrame coroutine that uses ScreenCapture, which | ||
| // captures the fully-composited game view (including UI Toolkit overlays). | ||
| // First call: queues the capture and returns "pending". | ||
| // Second call: result is ready – save PNG and return data. | ||
| if (Application.isPlaying) | ||
| { | ||
| // Build the output paths (used by both the pending and ready branches) | ||
| string resolvedPlayName = string.IsNullOrWhiteSpace(fileName) | ||
| ? $"ui-render-{DateTime.Now:yyyyMMdd-HHmmss}.png" | ||
| : fileName.Trim(); | ||
| if (!resolvedPlayName.EndsWith(".png", StringComparison.OrdinalIgnoreCase)) | ||
| resolvedPlayName += ".png"; | ||
|
|
||
| string playFolder = Path.Combine(Application.dataPath, "Screenshots"); | ||
| Directory.CreateDirectory(playFolder); | ||
| string playFullPath = Path.Combine(playFolder, resolvedPlayName).Replace('\\', '/'); | ||
| playFullPath = EnsureUniqueFilePath(playFullPath); | ||
| string playAssetsRelPath = "Assets/Screenshots/" + Path.GetFileName(playFullPath); | ||
|
|
||
| // ── Case 1: capture is ready ────────────────────────────────────── | ||
| if (s_pendingCaptureDone && s_pendingCaptureTex != null) | ||
| { | ||
| var captureTex = s_pendingCaptureTex; | ||
| s_pendingCaptureDone = false; | ||
| s_pendingCaptureTex = null; | ||
|
|
||
| int captureW = captureTex.width; | ||
| int captureH = captureTex.height; | ||
| byte[] capturePng = captureTex.EncodeToPNG(); | ||
| UnityEngine.Object.DestroyImmediate(captureTex); | ||
|
|
||
| File.WriteAllBytes(playFullPath, capturePng); | ||
| AssetDatabase.ImportAsset(playAssetsRelPath, ImportAssetOptions.ForceSynchronousImport); | ||
|
|
||
| var playData = new Dictionary<string, object> | ||
| { | ||
| { "path", playAssetsRelPath }, | ||
| { "fullPath", playFullPath }, | ||
| { "width", captureW }, | ||
| { "height", captureH }, | ||
| { "hasContent", true }, | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(target)) playData["gameObject"] = target; | ||
| if (!string.IsNullOrEmpty(uxmlPath)) playData["sourceAsset"] = uxmlPath; | ||
|
|
||
| if (includeImage) | ||
| { | ||
| int targetMax = maxResolution > 0 ? maxResolution : 640; | ||
| Texture2D downscaled = null; | ||
| try | ||
| { | ||
| var fullTex = new Texture2D(captureW, captureH, TextureFormat.RGBA32, false); | ||
| fullTex.LoadImage(capturePng); | ||
| if (captureW > targetMax || captureH > targetMax) | ||
| { | ||
| downscaled = ScreenshotUtility.DownscaleTexture(fullTex, targetMax); | ||
| playData["imageBase64"] = Convert.ToBase64String(downscaled.EncodeToPNG()); | ||
| playData["imageWidth"] = downscaled.width; | ||
| playData["imageHeight"] = downscaled.height; | ||
| } | ||
| else | ||
| { | ||
| playData["imageBase64"] = Convert.ToBase64String(capturePng); | ||
| playData["imageWidth"] = captureW; | ||
| playData["imageHeight"] = captureH; | ||
| } | ||
| UnityEngine.Object.DestroyImmediate(fullTex); | ||
| } | ||
| finally | ||
| { | ||
| if (downscaled != null) UnityEngine.Object.DestroyImmediate(downscaled); | ||
| } | ||
| } | ||
|
|
||
| return new SuccessResponse($"UI render saved to '{playAssetsRelPath}'.", playData); | ||
| } | ||
|
|
||
| // ── Case 2: start a new capture ─────────────────────────────────── | ||
| if (!s_pendingCaptureStarted) | ||
| { | ||
| s_pendingCaptureDone = false; | ||
| s_pendingCaptureTex = null; | ||
| s_pendingCaptureStarted = true; | ||
| var captureGo = new GameObject("__MCP_ScreenCapturer__") | ||
| { | ||
| hideFlags = HideFlags.HideAndDontSave | ||
| }; | ||
| captureGo.AddComponent<MCP_ScreenCapturer>(); | ||
| } | ||
|
|
||
| return new SuccessResponse( | ||
| "Play-mode screenshot capture queued (WaitForEndOfFrame). Call render_ui again to retrieve the rendered image.", | ||
| new Dictionary<string, object> | ||
| { | ||
| { "pending", true }, | ||
| { "gameObject", (object)target ?? uxmlPath }, | ||
| { "note", "A screen capture was scheduled for the end of this frame. Call render_ui once more to get the result." } | ||
| }); | ||
| } | ||
| // ── End play-mode branch ──────────────────────────────────────────────── | ||
|
|
||
| // Resolve UIDocument | ||
| UIDocument uiDoc = null; | ||
| GameObject tempGo = null; | ||
| PanelSettings tempPs = null; | ||
|
|
||
| try | ||
| { | ||
| if (!string.IsNullOrEmpty(target)) | ||
| { | ||
| var goInstruction = new JObject { ["find"] = target }; | ||
| GameObject go = ObjectResolver.Resolve(goInstruction, typeof(GameObject)) as GameObject; | ||
| if (go == null) | ||
| return new ErrorResponse($"Could not find target GameObject: {target}"); | ||
|
|
||
| uiDoc = go.GetComponent<UIDocument>(); | ||
| if (uiDoc == null) | ||
| return new ErrorResponse($"GameObject '{go.name}' has no UIDocument component."); | ||
| } | ||
| else | ||
| { | ||
| uxmlPath = AssetPathUtility.SanitizeAssetPath(uxmlPath); | ||
| if (uxmlPath == null) | ||
| return new ErrorResponse("Invalid UXML path."); | ||
|
|
||
| var vta = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(uxmlPath); | ||
| if (vta == null) | ||
| return new ErrorResponse($"Could not load VisualTreeAsset at: {uxmlPath}"); | ||
|
|
||
| tempGo = new GameObject("__MCP_UI_Render_Temp__"); | ||
| tempGo.hideFlags = HideFlags.HideAndDontSave; | ||
| uiDoc = tempGo.AddComponent<UIDocument>(); | ||
|
|
||
| string[] guids = AssetDatabase.FindAssets("t:PanelSettings"); | ||
| PanelSettings ps = null; | ||
| if (guids.Length > 0) | ||
| ps = AssetDatabase.LoadAssetAtPath<PanelSettings>(AssetDatabase.GUIDToAssetPath(guids[0])); | ||
| if (ps == null) | ||
| { | ||
| ps = CreateDefaultPanelSettings("Assets/UI/DefaultPanelSettings.asset"); | ||
| tempPs = ps; | ||
| } | ||
|
|
||
| uiDoc.panelSettings = ps; | ||
| uiDoc.visualTreeAsset = vta; | ||
| } | ||
|
|
||
| if (uiDoc.panelSettings == null) | ||
| return new ErrorResponse("UIDocument has no PanelSettings assigned."); | ||
|
|
||
| var panelSettings = uiDoc.panelSettings; | ||
| int psId = panelSettings.GetInstanceID(); | ||
|
|
||
| // Check if we already have a persistent RT assigned to this PanelSettings. | ||
| // If the RT exists and its size matches, the panel has been rendering into it. | ||
| // If not, create one and assign it — content will be available on the next call. | ||
| bool rtJustCreated = false; | ||
| RenderTexture rt = panelSettings.targetTexture as RenderTexture; | ||
|
|
||
| if (rt != null && s_panelRTs.ContainsKey(psId) && rt.width == width && rt.height == height) | ||
| { | ||
| // RT already assigned and size matches — panel has been rendering into it. | ||
| // We will read it below, then restore targetTexture = null so the UI | ||
| // goes back to rendering on the actual display/camera. | ||
| } | ||
| else | ||
| { | ||
| // Clean up old RT if size changed | ||
| if (s_panelRTs.TryGetValue(psId, out var oldRt) && oldRt != null) | ||
| { | ||
| panelSettings.targetTexture = null; | ||
| string oldPath = AssetDatabase.GetAssetPath(oldRt); | ||
| oldRt.Release(); | ||
| if (!string.IsNullOrEmpty(oldPath)) | ||
| AssetDatabase.DeleteAsset(oldPath); | ||
| else | ||
| UnityEngine.Object.DestroyImmediate(oldRt); | ||
| s_panelRTs.Remove(psId); | ||
| } | ||
|
|
||
| // Create RT as an asset so PanelSettings can serialize the reference properly | ||
| rt = new RenderTexture(width, height, 32, RenderTextureFormat.ARGB32); | ||
| rt.name = $"MCP_UI_Render_{psId}"; | ||
| rt.Create(); | ||
|
|
||
| string rtFolder = "Assets/UI"; | ||
| if (!AssetDatabase.IsValidFolder(rtFolder)) | ||
| AssetDatabase.CreateFolder("Assets", "UI"); | ||
| string rtAssetPath = $"{rtFolder}/RT_MCP_UI_Render_{psId}.renderTexture"; | ||
| AssetDatabase.CreateAsset(rt, rtAssetPath); | ||
| AssetDatabase.SaveAssets(); | ||
|
|
||
| panelSettings.targetTexture = rt; | ||
| s_panelRTs[psId] = rt; | ||
| rtJustCreated = true; |
There was a problem hiding this comment.
The render_ui function creates persistent RenderTextures that are stored in the static s_panelRTs dictionary and reused across calls. However, there's no cleanup mechanism to release these RenderTextures when they're no longer needed (e.g., when the editor is closed or when a PanelSettings is deleted). This could lead to memory leaks over time. Consider implementing a cleanup mechanism, such as subscribing to EditorApplication.quitting or checking if PanelSettings instances are still valid before reusing cached RenderTextures.
Contributor
Reviewer's GuideAdds a new manage_ui MCP tool and Unity editor implementation to manage UI Toolkit (UXML/USS, UIDocument, PanelSettings, visual trees, and screenshots), exposes it through the server tool registry, and updates documentation and tests so UI Toolkit becomes the preferred UI workflow alongside legacy uGUI. Sequence diagram for manage_ui render_ui play-mode capturesequenceDiagram
actor User
participant Client as MCP_Client
participant Server as manage_ui_server_tool
participant Transport as Unity_transport
participant Editor as Unity_Editor
participant ManageUI as ManageUI_HandleCommand
participant Capturer as MCP_ScreenCapturer
User->>Client: Call manage_ui(action=render_ui, target=GO)
Client->>Server: manage_ui request
Server->>Transport: send_mutation(manage_ui, params)
Transport->>Editor: MCP command manage_ui
Editor->>ManageUI: HandleCommand(render_ui)
alt first_call_in_play_mode
ManageUI->>Capturer: Create MCP_ScreenCapturer GameObject
ManageUI-->>Editor: Success(pending=true)
Editor-->>Transport: response
Transport-->>Server: response
Server-->>Client: pending=true, note
else second_call_after_frame
Capturer->>Capturer: WaitForEndOfFrame
Capturer->>ManageUI: Set pendingCaptureTex, pendingCaptureDone=true
User->>Client: Call manage_ui(action=render_ui, target=GO) again
Client->>Server: manage_ui request
Server->>Transport: send_mutation(manage_ui, params)
Transport->>Editor: MCP command manage_ui
Editor->>ManageUI: HandleCommand(render_ui)
ManageUI->>ManageUI: Encode PNG, save Assets/Screenshots
ManageUI-->>Editor: Success(path, hasContent=true, optional imageBase64)
Editor-->>Transport: response
Transport-->>Server: response
Server-->>Client: final screenshot info
end
Class diagram for ManageUI editor tool structureclassDiagram
class ManageUI {
<<static>>
-ValidExtensions : HashSet~string~
-s_panelRTs : Dictionary~int, RenderTexture~
-s_pendingCaptureTex : Texture2D
-s_pendingCaptureDone : bool
-s_pendingCaptureStarted : bool
+HandleCommand(params : JObject) object
-CreateFile(params : JObject) object
-ReadFile(params : JObject) object
-UpdateFile(params : JObject) object
-DeleteFile(params : JObject) object
-AttachUIDocument(params : JObject) object
-DetachUIDocument(params : JObject) object
-CreatePanelSettings(params : JObject) object
-UpdatePanelSettings(params : JObject) object
-ListUIAssets(params : JObject) object
-GetVisualTree(params : JObject) object
-ModifyVisualElement(params : JObject) object
-RenderUI(params : JObject) object
-LinkStylesheet(params : JObject) object
-ValidatePath(path : string, error : out string) string
-CreateDefaultPanelSettings(path : string) PanelSettings
-ApplyPanelSettingsProperties(ps : PanelSettings, settings : JObject, changes : List~string~) void
-ApplyDynamicAtlasSettings(ps : PanelSettings, da : JObject, changes : List~string~) void
-SerializeVisualElement(element : VisualElement, depth : int, maxDepth : int) object
-ApplyInlineStyles(element : VisualElement, styleObj : JObject, modifications : List~string~) void
-EnsureFolderExists(assetFolderPath : string) void
-FindUxmlBodyStart(content : string) int
-EnsureUniqueFilePath(path : string) string
-ColorToHex(c : Color) string
-GetDecodedContents(p : ToolParams) string
-TryParseEnum~T~(token : JToken, result : out T) bool
-TryFloat(token : JToken, result : out float) bool
-TryInt(token : JToken, result : out int) bool
-TryParseColor(token : JToken, color : out Color) bool
-NormalizeKey(key : string) string
}
class MCP_ScreenCapturer {
+Start() IEnumerator
}
ManageUI ..> MCP_ScreenCapturer : creates
ManageUI ..> UIDocument
ManageUI ..> PanelSettings
ManageUI ..> VisualTreeAsset
ManageUI ..> VisualElement
ManageUI ..> ThemeStyleSheet
ManageUI ..> RenderTexture
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The
ManageUIeditor tool has grown quite large (file ops, panel settings, visual tree inspection, render pipeline, element mutation) – consider splitting it into smaller, focused helpers/partials (e.g., file management, panel settings, rendering, element modification) to keep it easier to reason about and evolve. - In the server
manage_uitool, path validation only runs forcreate/read/update/delete, but other actions that acceptpath(likelink_stylesheetandrender_uiwith a UXML path) bypass these checks; it would be safer and more consistent to apply the same Assets/extension/traversal validation to all path-using actions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ManageUI` editor tool has grown quite large (file ops, panel settings, visual tree inspection, render pipeline, element mutation) – consider splitting it into smaller, focused helpers/partials (e.g., file management, panel settings, rendering, element modification) to keep it easier to reason about and evolve.
- In the server `manage_ui` tool, path validation only runs for `create/read/update/delete`, but other actions that accept `path` (like `link_stylesheet` and `render_ui` with a UXML path) bypass these checks; it would be safer and more consistent to apply the same Assets/extension/traversal validation to all path-using actions.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="125-126" />
<code_context>
+ return new ErrorResponse("'contents' parameter is required for create.");
+ }
+
+ string fullPath = Path.Combine(Application.dataPath,
+ path.Substring("Assets/".Length));
+ fullPath = fullPath.Replace('/', Path.DirectorySeparatorChar);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against non-`Assets/` paths before using `Substring("Assets/".Length)`.
Multiple methods (CreateFile, ReadFile, UpdateFile, DeleteFile, LinkStylesheet) assume `path` starts with `"Assets/"` and call `path.Substring("Assets/".Length)` without checking. Since `ValidatePath` doesn’t enforce this prefix, a value like `"UI/Main.uxml"` will throw `ArgumentOutOfRangeException`. Please either enforce and normalize the `Assets/` prefix in `ValidatePath`, or add an explicit prefix check before calling `Substring` and return a controlled error if it’s missing.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="238" />
<code_context>
+ }
+
+ // Load or create PanelSettings
+ string panelSettingsPath = p.Get("panel_settings");
+ PanelSettings panelSettings = null;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Parameter name mismatch with Python tool for `panel_settings` will prevent explicit PanelSettings selection.
The client wrapper (`manage_ui.py`) sends `panelSettings` (`params_dict["panelSettings"] = panel_settings`), but this code only reads `panel_settings`. This means explicit PanelSettings paths are ignored and the code always uses the auto-detection/creation path. Please either support both keys (e.g. `p.Get("panel_settings") ?? p.Get("panelSettings")`) or change this parameter name to match the client.
</issue_to_address>
### Comment 3
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="325-334" />
<code_context>
+ PanelSettings ps = null;
+ if (guids.Length > 0)
+ ps = AssetDatabase.LoadAssetAtPath<PanelSettings>(AssetDatabase.GUIDToAssetPath(guids[0]));
+ if (ps == null)
+ {
+ ps = CreateDefaultPanelSettings("Assets/UI/DefaultPanelSettings.asset");
+ tempPs = ps;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Destroying `tempPs` after creating it as an asset may leave an orphaned asset instance.
In the UXML-only `render_ui` path, `CreateDefaultPanelSettings` both creates and registers an asset, but the `finally` block calls `DestroyImmediate(tempPs, true)`. This destroys the instance while leaving the asset file behind. If this is meant to be temporary, prefer a non-asset `ScriptableObject`; if it’s meant to persist, don’t destroy it here, or explicitly remove it with `AssetDatabase.DeleteAsset` as part of cleanup.
</issue_to_address>
### Comment 4
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="1474-1476" />
<code_context>
+ }
+ break;
+
+ case "fontsize":
+ case "font-size":
+ element.style.fontSize = val.ToObject<float>();
+ modifications.Add($"fontSize={val}");
+ break;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unvalidated `ToObject<float>()` calls in inline style handling may throw for string or malformed values.
In `ApplyInlineStyles`, the direct `val.ToObject<float>()` calls (for `fontSize`, `width`, `height`, margins, etc.) will throw if the value is a string (e.g., "24" / "24px") or otherwise malformed JSON, aborting the whole change. Consider a helper like the existing `TryFloat` in `ApplyPanelSettingsProperties` and only set the style when parsing succeeds, optionally logging skipped fields in `modifications`.
Suggested implementation:
```csharp
case "fontsize":
case "font-size":
if (TryFloat(val, out float fontSize))
{
element.style.fontSize = fontSize;
modifications.Add($"fontSize={fontSize}");
}
else
{
modifications.Add($"fontSize_skipped={val}");
}
break;
```
1. Ensure that `TryFloat(JToken token, out float value)` is accessible from `ApplyInlineStyles` (e.g., it should be `private static` on the same class or otherwise visible). If it currently only exists in another helper/partial, move it to a shared location or make it available here.
2. For consistency with this change and your review comment, consider applying the same `TryFloat` pattern to other inline numeric style handlers in `ApplyInlineStyles` (e.g., `width`, `height`, margin/padding properties), including adding a `*_skipped` entry to `modifications` when parsing fails.
</issue_to_address>
### Comment 5
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="1645-1654" />
<code_context>
+ {
+ bool isEncoded = p.GetBool("contents_encoded") || p.GetBool("contentsEncoded");
+
+ if (isEncoded)
+ {
+ string encoded = p.Get("encoded_contents") ?? p.Get("encodedContents");
+ if (!string.IsNullOrEmpty(encoded))
+ {
+ try
+ {
+ return Encoding.UTF8.GetString(Convert.FromBase64String(encoded));
+ }
+ catch (FormatException)
+ {
+ return null;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Invalid base64 in `encodedContents` is treated as missing contents, which hides the real error.
The base64 decode `FormatException` in `GetDecodedContents` currently results in `null`, which then surfaces as a generic "'contents' parameter is required" error in `CreateFile`/`UpdateFile`. Consider distinguishing invalid base64 from missing contents (e.g., via a sentinel value or custom exception) so callers receive a specific "invalid encodedContents" error instead of a misleading missing-parameter message.
Suggested implementation:
```csharp
bool isEncoded = p.GetBool("contents_encoded") || p.GetBool("contentsEncoded");
if (isEncoded)
{
string encoded = p.Get("encoded_contents") ?? p.Get("encodedContents");
if (!string.IsNullOrEmpty(encoded))
{
try
{
return Encoding.UTF8.GetString(Convert.FromBase64String(encoded));
}
catch (FormatException ex)
{
// Distinguish invalid base64 from missing contents so callers can surface
// a clear "invalid encodedContents" error instead of a generic missing-parameter message.
throw new ArgumentException(
"Parameter 'encodedContents' must be valid base64 when 'contentsEncoded' is true.",
ex);
```
Depending on how errors are propagated in your tool framework, you may want to:
1. Ensure that the exception message from `GetDecodedContents` is surfaced directly to the user (or mapped to your tool error format) rather than being wrapped in a generic error.
2. Remove or adjust any caller logic that treats `null` from `GetDecodedContents` as "missing contents" so it doesn't attempt to handle the invalid-base64 case as if it were a missing parameter.
</issue_to_address>
### Comment 6
<location path="TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs" line_range="406-409" />
<code_context>
+ // ---- Delete file ----
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Add tests for path traversal protection and Assets-root enforcement in editor-side file operations.
On the C# side, `ValidatePath` rejects traversal sequences and enforces `.uxml`/`.uss` extensions, but the tests only cover invalid extensions. Please add `HandleCommand` tests for `create/read/update/delete` that use traversal-like paths (e.g. `Assets/../foo.uxml`) and paths outside `Assets/`, and assert that `success` is false and the error reports an invalid/traversal path to exercise this validation.
```suggestion
// ---- Delete file ----
[TestCase("Assets/../Traversal_Create.uxml")]
[TestCase("OutsideRoot/Traversal_Create.uxml")]
public void Create_WithInvalidOrTraversalPath_FailsValidation(string path)
{
var result = ManageUI.HandleCommand(new JObject
{
["action"] = "create",
["path"] = path,
["contents"] = "<uxml />"
});
Assert.IsFalse(result.Value<bool>("success"));
Assert.That(result["error"].ToString(), Does.Contain("invalid").IgnoreCase);
}
[TestCase("Assets/../Traversal_Read.uxml")]
[TestCase("OutsideRoot/Traversal_Read.uxml")]
public void Read_WithInvalidOrTraversalPath_FailsValidation(string path)
{
var result = ManageUI.HandleCommand(new JObject
{
["action"] = "read",
["path"] = path,
});
Assert.IsFalse(result.Value<bool>("success"));
Assert.That(result["error"].ToString(), Does.Contain("invalid").IgnoreCase);
}
[TestCase("Assets/../Traversal_Update.uxml")]
[TestCase("OutsideRoot/Traversal_Update.uxml")]
public void Update_WithInvalidOrTraversalPath_FailsValidation(string path)
{
var result = ManageUI.HandleCommand(new JObject
{
["action"] = "update",
["path"] = path,
["contents"] = "<uxml />"
});
Assert.IsFalse(result.Value<bool>("success"));
Assert.That(result["error"].ToString(), Does.Contain("invalid").IgnoreCase);
}
[TestCase("Assets/../Traversal_Delete.uxml")]
[TestCase("OutsideRoot/Traversal_Delete.uxml")]
public void Delete_WithInvalidOrTraversalPath_FailsValidation(string path)
{
var result = ManageUI.HandleCommand(new JObject
{
["action"] = "delete",
["path"] = path,
});
Assert.IsFalse(result.Value<bool>("success"));
Assert.That(result["error"].ToString(), Does.Contain("invalid").IgnoreCase);
}
[Test]
public void Delete_ExistingFile_DeletesFile()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+125
to
+126
| string fullPath = Path.Combine(Application.dataPath, | ||
| path.Substring("Assets/".Length)); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against non-Assets/ paths before using Substring("Assets/".Length).
Multiple methods (CreateFile, ReadFile, UpdateFile, DeleteFile, LinkStylesheet) assume path starts with "Assets/" and call path.Substring("Assets/".Length) without checking. Since ValidatePath doesn’t enforce this prefix, a value like "UI/Main.uxml" will throw ArgumentOutOfRangeException. Please either enforce and normalize the Assets/ prefix in ValidatePath, or add an explicit prefix check before calling Substring and return a controlled error if it’s missing.
Comment on lines
+1474
to
+1476
| case "fontsize": | ||
| case "font-size": | ||
| element.style.fontSize = val.ToObject<float>(); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Unvalidated ToObject<float>() calls in inline style handling may throw for string or malformed values.
In ApplyInlineStyles, the direct val.ToObject<float>() calls (for fontSize, width, height, margins, etc.) will throw if the value is a string (e.g., "24" / "24px") or otherwise malformed JSON, aborting the whole change. Consider a helper like the existing TryFloat in ApplyPanelSettingsProperties and only set the style when parsing succeeds, optionally logging skipped fields in modifications.
Suggested implementation:
case "fontsize":
case "font-size":
if (TryFloat(val, out float fontSize))
{
element.style.fontSize = fontSize;
modifications.Add($"fontSize={fontSize}");
}
else
{
modifications.Add($"fontSize_skipped={val}");
}
break;- Ensure that
TryFloat(JToken token, out float value)is accessible fromApplyInlineStyles(e.g., it should beprivate staticon the same class or otherwise visible). If it currently only exists in another helper/partial, move it to a shared location or make it available here. - For consistency with this change and your review comment, consider applying the same
TryFloatpattern to other inline numeric style handlers inApplyInlineStyles(e.g.,width,height, margin/padding properties), including adding a*_skippedentry tomodificationswhen parsing fails.
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
ManageUIeditor tool has grown quite large (single 1600+ line class); consider splitting it into smaller helpers/partial classes (e.g., file ops, panel settings, visual tree/rendering, live element mutation) to keep it easier to navigate and maintain. - In the Python
manage_uitool, path validation is only applied for create/read/update/delete; ifpathis used with actions likelink_stylesheetorrender_ui, you may want to apply the sameAssets/+ traversal checks there to fail fast before hitting Unity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ManageUI` editor tool has grown quite large (single 1600+ line class); consider splitting it into smaller helpers/partial classes (e.g., file ops, panel settings, visual tree/rendering, live element mutation) to keep it easier to navigate and maintain.
- In the Python `manage_ui` tool, path validation is only applied for create/read/update/delete; if `path` is used with actions like `link_stylesheet` or `render_ui`, you may want to apply the same `Assets/` + traversal checks there to fail fast before hitting Unity.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="87-96" />
<code_context>
+ private static string ValidatePath(string path, out string error)
</code_context>
<issue_to_address>
**issue (bug_risk):** ValidatePath assumes an 'Assets/' prefix and can throw on other valid paths.
`ValidatePath` calls `path.Substring("Assets/".Length)` without verifying that the sanitized path actually starts with `"Assets/"`. For sanitized paths outside `Assets/` (different casing/missing prefix), this will throw `ArgumentOutOfRangeException` instead of returning a controlled error. Please add an explicit (ideally case-insensitive) `StartsWith("Assets/")` check and return a clear error before calling `Substring`.
Since this pattern appears in multiple callers (CreateFile/ReadFile/UpdateFile/DeleteFile/LinkStylesheet), centralizing the prefix validation inside `ValidatePath` would improve robustness and consistency.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="457-458" />
<code_context>
+ if (TryFloat(val, out float fbDpi)) { ps.fallbackDpi = fbDpi; changes.Add("fallbackDpi"); }
+ break;
+
+ case "sortingorder":
+ if (TryFloat(val, out float so)) { ps.sortingOrder = so; changes.Add("sortingOrder"); }
+ break;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** PanelSettings.sortingOrder is an int but is being assigned from a float helper.
`sortingorder` currently uses `TryFloat` and assigns the result to `ps.sortingOrder`, which is an `int`. This is either a compile error or an implicit narrowing conversion. Please treat this as an integer instead, consistent with `targetDisplay`:
```csharp
case "sortingorder":
if (TryInt(val, out int so)) { ps.sortingOrder = so; changes.Add("sortingOrder"); }
break;
```
</issue_to_address>
### Comment 3
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="824-833" />
<code_context>
+
+ return new SuccessResponse(msg, data);
+ }
+ finally
+ {
+ if (tempGo != null) UnityEngine.Object.DestroyImmediate(tempGo);
+ if (tempPs != null) UnityEngine.Object.DestroyImmediate(tempPs, true);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Destroying a PanelSettings asset instance created on disk can leave a dangling asset.
In `RenderUI`’s `finally` block you call `DestroyImmediate(tempPs, true)` even though `tempPs` was just created via `CreateDefaultPanelSettings("Assets/UI/DefaultPanelSettings.asset")`, which writes a real asset to disk and registers it with the `AssetDatabase`.
Destroying that instance can leave the `.asset` file on disk pointing to a missing/invalid object, which may confuse the editor or future loads. Either keep the persisted default `PanelSettings` asset alive (don’t destroy it), or create a truly transient `PanelSettings` for the render path (no `AssetDatabase.CreateAsset`), and only destroy non-persisted instances.
</issue_to_address>
### Comment 4
<location path="MCPForUnity/Editor/Tools/ManageUI.cs" line_range="722-733" />
<code_context>
+ private System.Collections.IEnumerator Start()
+ {
+ yield return new WaitForEndOfFrame();
+ ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture();
+ ManageUI.s_pendingCaptureDone = true;
+ ManageUI.s_pendingCaptureStarted = false;
</code_context>
<issue_to_address>
**suggestion:** Play-mode ScreenCapture usage isn’t guarded by the screen-capture capability flag.
`MCP_ScreenCapturer.Start()` always calls `ScreenCapture.CaptureScreenshotAsTexture()` even though you expose `screenCapture = ScreenshotUtility.IsScreenCaptureModuleAvailable` in `ProjectInfo`.
On platforms/configurations where screen capture is unavailable or disabled, this may throw or behave unpredictably while still advertising the capability.
Consider either checking `ScreenshotUtility.IsScreenCaptureModuleAvailable` (or similar) before performing the capture and failing gracefully, or only exposing `screenCapture` as true when this API is actually supported at runtime, so play-mode behavior matches the capability flag.
```suggestion
// MonoBehaviour that captures a screenshot at end-of-frame in play mode.
private sealed class MCP_ScreenCapturer : MonoBehaviour
{
private System.Collections.IEnumerator Start()
{
// Guard play-mode capture with the same capability used for project info,
// so behavior matches the advertised screen-capture support.
if (!ScreenshotUtility.IsScreenCaptureModuleAvailable)
{
// Fail gracefully: mark capture as "done" with no texture
// and clear the "started" flag so callers can detect the failure.
ManageUI.s_pendingCaptureTex = null;
ManageUI.s_pendingCaptureDone = true;
ManageUI.s_pendingCaptureStarted = false;
Destroy(gameObject);
yield break;
}
yield return new WaitForEndOfFrame();
ManageUI.s_pendingCaptureTex = ScreenCapture.CaptureScreenshotAsTexture();
ManageUI.s_pendingCaptureDone = true;
ManageUI.s_pendingCaptureStarted = false;
Destroy(gameObject);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+87
to
+96
| private static string ValidatePath(string path, out string error) | ||
| { | ||
| error = null; | ||
| if (string.IsNullOrEmpty(path)) | ||
| { | ||
| error = "'path' parameter is required."; | ||
| return null; | ||
| } | ||
|
|
||
| path = AssetPathUtility.SanitizeAssetPath(path); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): ValidatePath assumes an 'Assets/' prefix and can throw on other valid paths.
ValidatePath calls path.Substring("Assets/".Length) without verifying that the sanitized path actually starts with "Assets/". For sanitized paths outside Assets/ (different casing/missing prefix), this will throw ArgumentOutOfRangeException instead of returning a controlled error. Please add an explicit (ideally case-insensitive) StartsWith("Assets/") check and return a clear error before calling Substring.
Since this pattern appears in multiple callers (CreateFile/ReadFile/UpdateFile/DeleteFile/LinkStylesheet), centralizing the prefix validation inside ValidatePath would improve robustness and consistency.
Add lifecycle cleanup for panel RenderTextures on editor quit / assembly reload to avoid leaked assets; implement CleanupRenderTextures. Harden play-mode screenshot flow: check ScreenshotUtility availability, catch CaptureScreenshot failures, and reject concurrent captures with a clear error. Improve error handling for decoded contents (throw/return ArgumentException message), accept panel_settings or panelSettings key, parse sortingOrder as int, and delete temporary PanelSettings assets created during operations. Robustify UXML parsing by skipping matches inside XML comments. Update tests: adjust a Python integration call and add multiple C# tests to verify path traversal validation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageUI.cs (1)
44-50: UseToolParamsforactionparsing to keep validation consistent.
HandleCommanddirectly reads@params["action"]while the rest of the tool usesToolParams. Please routeactionthroughToolParamsas well for consistent error behavior and type coercion patterns.As per coding guidelines:
MCPForUnity/Editor/Tools/*.cs: UseToolParamsfor consistent parameter validation in C# Editor tools with methods likeGetInt(),RequireString().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageUI.cs` around lines 44 - 50, Replace direct JObject access in HandleCommand with ToolParams: instantiate a ToolParams from the incoming JObject (e.g., new ToolParams(`@params`)) and call its RequireString/GetString method to obtain the "action" value, then ToLowerInvariant() for downstream switch logic; this keeps validation and error behavior consistent with other tools (preserving ErrorResponse semantics) and uses the ToolParams API instead of direct `@params`["action"] access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 1511-1514: The code treats "[skipped]" entries from
ApplyInlineStyles as successful modifications, causing false-positive success
responses; update ManageUI to treat only real changes as modifications by either
making ApplyInlineStyles append only actual changes (not "[skipped]" messages)
or by filtering the returned modifications list to remove entries that begin
with "[skipped]" before checking modifications.Count and before setting the
response; apply the same fix for the other check around the second call (the
block referenced near lines 1661-1663) so that only true modifications count
toward success.
- Around line 988-1007: The code is deleting cached RenderTextures because after
reading the RT you set panelSettings.targetTexture = null which makes the
subsequent check treat rt as unused and remove it; instead, change the flow in
the capture path to preserve the cached RT in s_panelRTs and restore
panelSettings.targetTexture after any temporary read operation (or use a
temporary duplicate/readback RT) so that rt != null remains true across calls;
update logic around psId/s_panelRTs and the cleanup block (the branch that
checks rt, s_panelRTs.ContainsKey(psId), rt.width/height and the else that
removes oldRt) to only release/remove the cached RT when its size truly differs
or it is explicitly disposed, not just because panelSettings.targetTexture was
momentarily set to null during capture.
In `@Server/tests/integration/test_manage_ui.py`:
- Line 39: Multiple monkeypatched stub functions named fake_send in
Server/tests/integration/test_manage_ui.py declare unused named parameters and
trigger Ruff ARG001; update each fake_send signature (e.g., the one currently
defined as async def fake_send(ctx, instance, cmd, params, **kwargs): and the
other occurrences listed) to either prefix unused parameter names with an
underscore (for example _instance, _cmd, _params) or replace unused
positional/named params with *args, **kwargs so linting no longer flags ARG001
while preserving call compatibility in the tests.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 44-50: Replace direct JObject access in HandleCommand with
ToolParams: instantiate a ToolParams from the incoming JObject (e.g., new
ToolParams(`@params`)) and call its RequireString/GetString method to obtain the
"action" value, then ToLowerInvariant() for downstream switch logic; this keeps
validation and error behavior consistent with other tools (preserving
ErrorResponse semantics) and uses the ToolParams API instead of direct
`@params`["action"] access.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageUI.csServer/tests/integration/test_manage_ui.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs
Scriptwonder
added a commit
that referenced
this pull request
Feb 26, 2026
This reverts commit 6ab1069.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This introduce a quicker way to initiate UI, as the uGUI way is proved to not be painful creation progress with the current LLM. Using a current tool, you can generate UIs based on UI toolkit directly and quickly modify them to your need.
Type of Change
Save your change type
Changes Made
Testing/Screenshots/Recordings
(And uGui is generally not good for creating screen sized UI, since LLM has not overcome the spatial reasoning challenge yet)
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Introduce a new manage_ui tool and UI Toolkit–first workflows for Unity MCP, enabling creation, management, and visualization of UI Toolkit-based interfaces alongside existing uGUI support.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests