Open and close prefabs in the stage view + create them #283
Conversation
This might be enough, but it's possible we may have to extract some logic from ManageGameObject
…ferences in ManageAsset and ManagePrefabs
…create_from_gameobject' action
… manage_prefabs tools
WalkthroughAdds a prefab-management tool and tests: implements editor commands to open/close/save prefab stages and create prefabs from GameObjects, adds an asset-path sanitizer, wires command dispatch to the bridge and Python server tooling, updates asset/menu handling to use the sanitizer and direct menu execution, and includes Unity edit-mode tests and meta assets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as MCP Client
participant Server as MCP Server (Python)
participant Bridge as Unity Bridge (C#)
participant Prefabs as ManagePrefabs
participant Unity as Unity Editor
User->>CLI: invoke manage_prefabs(action, params)
CLI->>Server: call manage_prefabs(...)
Server->>Bridge: send_command "manage_prefabs" with params
Bridge->>Prefabs: HandleCommand(params)
alt open_stage
Prefabs->>Unity: Load prefab asset & Open PrefabStage
Prefabs-->>Bridge: serialized stage info / error
else close_stage
Prefabs->>Unity: Get PrefabStage -> (save if requested) -> Close
Prefabs-->>Bridge: success / error
else save_open_stage
Prefabs->>Unity: Save PrefabStage -> Return info
Prefabs-->>Bridge: success / error
else create_from_gameobject
Prefabs->>Unity: Find GameObject -> Sanitize path -> Ensure dir -> SaveAsPrefabAsset -> Select instance
Prefabs-->>Bridge: { path, instanceId } / error
end
Bridge-->>Server: response { success, message, data? }
Server-->>CLI: standardized dict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (13)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (2)
11-21: Silence unusedctxargument (Ruff ARG001).Rename the param to
_ctxto keep the signature while avoiding the warning.Apply this diff:
- def manage_prefabs( - ctx: Context, + def manage_prefabs( + _ctx: Context,
58-59: Document intentional broad exception at the tool boundary (Ruff BLE001).Catching Exception is acceptable here to always return a structured error. Add a noqa to avoid lint churn.
Apply this diff:
- except Exception as exc: + except Exception as exc: # noqa: BLE001 - tool boundary; return structured error to client return {"success": False, "message": f"Python error managing prefabs: {exc}"}UnityMcpBridge/Editor/Tools/ManageAsset.cs (3)
160-166: Reuse EnsureDirectoryExists instead of manual IO.Avoid duplicating directory creation logic and rely on the existing helper.
Apply this diff:
- // Ensure directory exists - if (!Directory.Exists(Path.Combine(Directory.GetCurrentDirectory(), directory))) - { - Directory.CreateDirectory(Path.Combine(Directory.GetCurrentDirectory(), directory)); - AssetDatabase.Refresh(); // Make sure Unity knows about the new folder - } + // Ensure directory exists + EnsureDirectoryExists(directory);
283-327: Create parent folders reliably before CreateFolder.AssetDatabase.CreateFolder requires the parent to exist. Proactively ensure it.
Apply this diff:
- // Ensure parent exists - if (!string.IsNullOrEmpty(parentDir) && !AssetDatabase.IsValidFolder(parentDir)) - { - // Recursively create parent folders if needed (AssetDatabase handles this internally) - // Or we can do it manually: Directory.CreateDirectory(Path.Combine(Directory.GetCurrentDirectory(), parentDir)); AssetDatabase.Refresh(); - } + // Ensure parent exists + if (!string.IsNullOrEmpty(parentDir) && !AssetDatabase.IsValidFolder(parentDir)) + { + EnsureDirectoryExists(parentDir); + }
355-400: Support modifying components on child objects within prefabs.Current logic only targets components on the root. Consider allowing dotted or path-qualified keys (e.g., "Child/Sub:Collectible") to resolve nested targets.
If desired, I can draft a helper to parse "GameObjectPath:Component" and traverse Transform hierarchy before applying properties.
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (3)
128-208: Guard against creating prefabs from within an open prefab stage.When a prefab stage is open, FindSceneObjectByName searches the stage first; users might unintentionally create a new prefab from staged content. Consider rejecting create_from_gameobject if a prefab stage is open unless explicitly allowed.
I can draft a gate (e.g., reject when PrefabStageUtility.GetCurrentPrefabStage() != null unless @params["allowInPrefabStage"] == true).
210-224: Prefer AssetDatabase.CreateFolder over raw IO for directory creation.This ensures meta files are created deterministically.
Apply this diff:
- private static void EnsureAssetDirectoryExists(string assetPath) - { - string directory = Path.GetDirectoryName(assetPath); - if (string.IsNullOrEmpty(directory)) - { - return; - } - - string fullDirectory = Path.Combine(Directory.GetCurrentDirectory(), directory); - if (!Directory.Exists(fullDirectory)) - { - Directory.CreateDirectory(fullDirectory); - AssetDatabase.Refresh(); - } - } + private static void EnsureAssetDirectoryExists(string assetPath) + { + var directory = Path.GetDirectoryName(assetPath)?.Replace('\\', '/'); + if (string.IsNullOrEmpty(directory) || AssetDatabase.IsValidFolder(directory)) + return; + + var parts = directory.Split('/'); + var current = parts[0]; // "Assets" + for (int i = 1; i < parts.Length; i++) + { + var next = $"{current}/{parts[i]}"; + if (!AssetDatabase.IsValidFolder(next)) + { + AssetDatabase.CreateFolder(current, parts[i]); + } + current = next; + } + }
226-254: Allow hierarchical target selection to disambiguate.Name-only lookups are ambiguous. Consider supporting "Root/Child/SubChild" paths and, optionally, instanceID targeting.
I can provide a helper that resolves Transform by hierarchical path (including inactive) before falling back to name scan.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs (5)
29-37: Refresh after deleting temp folders.Call AssetDatabase.Refresh to keep the DB in sync post-delete.
Apply this diff:
public void CleanupAll() { StageUtility.GoToMainStage(); if (AssetDatabase.IsValidFolder(TempDirectory)) { AssetDatabase.DeleteAsset(TempDirectory); + AssetDatabase.Refresh(); } }
70-72: Refresh after deleting test assets.Ensures subsequent tests don’t see stale state.
Apply this diff:
StageUtility.GoToMainStage(); AssetDatabase.DeleteAsset(prefabPath); + AssetDatabase.Refresh();
110-112: Refresh after deleting test assets.Apply this diff:
StageUtility.GoToMainStage(); AssetDatabase.DeleteAsset(prefabPath); + AssetDatabase.Refresh();
200-206: Refresh after deleting created prefab.Apply this diff:
if (AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(prefabPath) != null) { AssetDatabase.DeleteAsset(prefabPath); + AssetDatabase.Refresh(); }
164-201: Add a test for allowOverwrite=False unique-path behavior.Verify that create_from_gameobject with an existing prefab path returns a generated unique path when allowOverwrite is false.
I can add a test method (mirroring CreateFromGameObject_CreatesPrefabAndLinksInstance) that calls the tool twice: first create the prefab, then re-run with the same path and assert savedPath != requested path and asset exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/unity-tests.yml(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/AssetPathUtility.cs(1 hunks)UnityMcpBridge/Editor/Helpers/AssetPathUtility.cs.meta(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(2 hunks)UnityMcpBridge/Editor/Tools/ManageAsset.cs(16 hunks)UnityMcpBridge/Editor/Tools/ManageEditor.cs(4 hunks)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs(0 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs(0 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs(0 hunks)UnityMcpBridge/Editor/Tools/Prefabs.meta(1 hunks)UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs(1 hunks)UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs.meta(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py(1 hunks)
💤 Files with no reviewable changes (3)
- UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs
- UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs
- UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#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:
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.csUnityMcpBridge/Editor/MCPForUnityBridge.cs
🧬 Code graph analysis (7)
UnityMcpBridge/Editor/Tools/ManageEditor.cs (1)
UnityMcpBridge/Editor/Helpers/Response.cs (3)
Response(10-61)Success(18-33)Error(41-60)
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (3)
UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)UnityMcpBridge/Editor/Helpers/McpLog.cs (1)
McpLog(6-30)UnityMcpBridge/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(8-28)SanitizeAssetPath(13-27)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(388-407)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
ManagePrefabs(12-273)
UnityMcpBridge/Editor/Tools/ManageAsset.cs (3)
UnityMcpBridge/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(8-28)SanitizeAssetPath(13-27)UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
Color(2020-2033)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (1)
Color(102-111)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py (2)
manage_prefabs(13-59)register_manage_prefabs_tools(8-59)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs (2)
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (3)
ManagePrefabs(12-273)HandleCommand(16-50)GameObject(226-254)UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
HandleCommand(29-149)ManageEditor(18-625)
🪛 Ruff (0.13.1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_prefabs.py
14-14: Unused function argument: ctx
(ARG001)
58-58: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (26)
.github/workflows/unity-tests.yml (1)
10-16: LGTM - PR trigger configuration aligns with push paths.The PR trigger configuration correctly mirrors the existing push trigger paths and includes appropriate event types for continuous integration testing.
UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
18-18: LGTM - Import correctly added for prefab management.The using directive for
MCPForUnity.Editor.Tools.Prefabsis properly positioned and aligns with the existing import structure.
1059-1059: LGTM - Command routing for prefab management properly integrated.The
manage_prefabscase is correctly added to the command dispatcher and follows the established pattern of routing toManagePrefabs.HandleCommand(paramsObject).UnityMcpBridge/Editor/Tools/Prefabs.meta (1)
1-9: Standard Unity .meta file - no issues.The metadata file follows Unity's standard format with appropriate values for a folder asset.
UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs.meta (1)
1-12: Standard Unity .meta file - no issues.The metadata file follows Unity's standard MonoImporter format with appropriate default values.
UnityMcpBridge/Editor/Helpers/AssetPathUtility.cs.meta (1)
1-12: Standard Unity .meta file - no issues.The metadata file follows Unity's standard MonoImporter format with appropriate default values.
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (2)
8-8: LGTM - Import follows established pattern.The import for
register_manage_prefabs_toolsis properly positioned alphabetically and follows the existing import structure.
26-26: LGTM - Tool registration properly integrated.The call to
register_manage_prefabs_tools(mcp)is correctly positioned alphabetically and follows the established registration pattern.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs.meta (1)
1-12: Standard Unity .meta file - no issues.The metadata file follows Unity's standard MonoImporter format with appropriate default values.
UnityMcpBridge/Editor/Tools/ManageEditor.cs (4)
8-8: LGTM - Imports properly added for prefab stage support.The added imports for
UnityEditor.SceneManagementandMCPForUnity.Editor.Helpersare necessary for the new prefab stage functionality and follow the existing import structure.Also applies to: 10-10
102-103: LGTM - New action properly integrated into command handler.The
get_prefab_stagecase is correctly added to the switch statement and routes to the appropriate handler method.
146-147: LGTM - Error message updated to include new action.The error message correctly includes the new
get_prefab_stageaction in the list of supported actions.
250-278: Well-implemented prefab stage information retrieval.The
GetPrefabStageInfomethod properly:
- Uses the correct Unity API (
PrefabStageUtility.GetCurrentPrefabStage())- Handles the null case when no prefab stage is open
- Returns comprehensive stage information including path, root name, mode, and dirty state
- Follows established error handling patterns
- Uses the standard
Response.SuccessandResponse.ErrorhelpersUnityMcpBridge/Editor/Tools/ManageAsset.cs (10)
118-121: LGTM: centralized path sanitation.Using AssetPathUtility improves consistency across asset operations.
341-352: LGTM: sanitize and validate before load.Correctly normalizes the path and guards asset existence.
541-546: LGTM: sanitize destination path and ensure directory exists.Prevents accidental writes outside Assets and ensures a valid target.
579-581: LGTM: consistent sanitization for move/rename.
645-655: LGTM: sanitized folder scope plus validity fallback.
735-744: LGTM: sanitized get_info path.
763-767: LGTM: sanitized get_components path.
921-947: Nice: accept Color as array input.The added JArray branch improves ergonomics and maintains safety.
983-984: LGTM: sanitize texture asset path before load.
1211-1223: LGTM: sanitize UnityObject load paths in converter.UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (3)
67-79: LGTM: validate mode and open prefab stage.Strictly supporting InIsolation avoids unexpected in-context edits.
90-99: Confirm close_stage behavior with unsaved changes (dialog risk).If saveBeforeClose is false and the stage is dirty, StageUtility.GoToMainStage may prompt the user, blocking automation. Consider auto-saving when dirty or adding a parameter to discard programmatically.
Do you see any modal prompts when calling close_stage with unsaved changes from the MCP path in an interactive editor session?
256-271: LGTM: stage serialization payload is concise and useful.
This reverts commit 10bfe54.
|
Ah, I can't enable unit tests from forks:
https://docs.github.com/en/code-security/getting-started/understanding-github-secret-types |
This brings the Unity window into focus but that seems to be a better UX for devs. Also streamline manage_menu_item tool info, as FastMCP recommends
Remove delayed execution of executing menu item, fixing CoplayDev#279
|
Since it was quick to do, it also fixes #279 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (1)
40-43: Drop the stale “via delayCall” wording.We no longer schedule through
delayCall, so the failure log should stop referencing it.- McpLog.Error($"[MenuItemExecutor] Failed to execute menu item via delayCall: '{menuPath}'. It might be invalid, disabled, or context-dependent."); + McpLog.Error($"[MenuItemExecutor] Failed to execute menu item '{menuPath}'. It might be invalid, disabled, or context-dependent.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (2)
UnityMcpBridge/Editor/Helpers/McpLog.cs (1)
McpLog(6-30)UnityMcpBridge/Editor/Helpers/Response.cs (1)
Response(10-61)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
16-16: Description metadata is a nice improvement.Adding the explicit description keeps this tool consistent with the newer registrations and improves discoverability in MCP tooling.
There was a problem hiding this comment.
Auto-updates the server-version.txt file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
1051-1053: Clarify timeout propagation for manage_scene
HandleManageScenecurrently converts a null result intoResponse.Error("manage_scene returned null..."), so the null-coalescing?? throw new TimeoutException(...)can never trigger. If you meant to surface timeouts as top-level errors, let the null bubble up (and throw here); otherwise drop the null-coalescing throw to keep the intent unambiguous.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#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:
UnityMcpBridge/Editor/MCPForUnityBridge.cs
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (9)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
ManageScript(52-2550)HandleCommand(111-290)UnityMcpBridge/Editor/Tools/ManageAsset.cs (2)
HandleCommand(45-110)ManageAsset(25-1329)UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
HandleCommand(29-149)ManageEditor(18-625)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (2)
HandleCommand(12-39)ManageMenuItem(7-40)UnityMcpBridge/Editor/Tools/ManageScene.cs (1)
HandleCommand(51-150)UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
HandleCommand(41-174)ManageGameObject(22-2192)UnityMcpBridge/Editor/Tools/ManageShader.cs (2)
HandleCommand(20-123)ManageShader(15-341)UnityMcpBridge/Editor/Tools/ReadConsole.cs (3)
HandleCommand(129-202)ReadConsole(17-569)ReadConsole(37-125)UnityMcpBridge/Editor/Tools/Prefabs/ManagePrefabs.cs (1)
ManagePrefabs(12-273)
* refactor: remove unused UnityEngine references from menu item classes * Add new tools to manage a prefab, particularly, making them staged. This might be enough, but it's possible we may have to extract some logic from ManageGameObject * feat: add AssetPathUtility for asset path normalization and update references in ManageAsset and ManagePrefabs * feat: add prefab management tools and register them with the MCP server * feat: update prefab management commands to use 'prefabPath' and add 'create_from_gameobject' action * fix: update parameter references to 'prefabPath' in ManagePrefabs and manage_prefabs tools * fix: clarify error message for missing 'prefabPath' in create_from_gameobject command * fix: ensure pull request triggers for unity tests workflow * Revert "fix: ensure pull request triggers for unity tests workflow" This reverts commit 10bfe54. * Remove delayed execution of executing menu item, fixing CoplayDev#279 This brings the Unity window into focus but that seems to be a better UX for devs. Also streamline manage_menu_item tool info, as FastMCP recommends * docs: clarify menu item tool description with guidance to use list action first * feat: add version update for server_version.txt in bump-version workflow * fix: simplify error message for failed menu item execution
This closes #215. I think it's worth re-evaluating how we interact with game object hierarchies before fully supporting #97, it's a good bit of code changes so I've kept this PR limited to prefab stages + creating them.
Can one of you have a look at the changes?
Summary by CodeRabbit