feat: add open prefab stage editor action#947
Open
jiajunfeng wants to merge 1 commit intoCoplayDev:betafrom
Open
feat: add open prefab stage editor action#947jiajunfeng wants to merge 1 commit intoCoplayDev:betafrom
jiajunfeng wants to merge 1 commit intoCoplayDev:betafrom
Conversation
Contributor
Reviewer's GuideAdds a new manage_editor action to open prefab assets in Prefab Stage, wires it through the Python MCP tool surface and documentation, and adds both Unity edit-mode and Python tests (plus missing meta file) to cover the new behavior. Sequence diagram for manage_editor open_prefab_stage flowsequenceDiagram
actor User
participant LLMClient
participant ServerManageEditorTool
participant UnityConnection
participant UnityManageEditor
participant AssetDatabase
participant PrefabStageUtility
User->>LLMClient: Request to edit prefab by path
LLMClient->>ServerManageEditorTool: Call manage_editor action=open_prefab_stage prefab_path
ServerManageEditorTool->>UnityConnection: async_send_command_with_retry action=open_prefab_stage prefabPath path
UnityConnection->>UnityManageEditor: HandleCommand(params)
UnityManageEditor->>UnityManageEditor: Extract action prefabPath path
UnityManageEditor->>UnityManageEditor: Select case open_prefab_stage
UnityManageEditor->>UnityManageEditor: OpenPrefabStage(prefabPath)
UnityManageEditor->>UnityManageEditor: Validate prefabPath not empty
UnityManageEditor->>UnityManageEditor: AssetPathUtility.SanitizeAssetPath
UnityManageEditor->>UnityManageEditor: Check path under Assets and ends with .prefab
UnityManageEditor->>AssetDatabase: LoadAssetAtPath GameObject
AssetDatabase-->>UnityManageEditor: prefabAsset or null
alt prefabAsset null
UnityManageEditor-->>UnityConnection: ErrorResponse prefab asset not found
else prefabAsset found
UnityManageEditor->>AssetDatabase: OpenAsset prefabAsset
AssetDatabase-->>UnityManageEditor: opened flag
UnityManageEditor->>PrefabStageUtility: GetCurrentPrefabStage
PrefabStageUtility-->>UnityManageEditor: prefabStage
UnityManageEditor->>UnityManageEditor: Determine enteredPrefabStage
alt not opened and not enteredPrefabStage
UnityManageEditor-->>UnityConnection: ErrorResponse failed to open prefab stage
else opened or enteredPrefabStage
UnityManageEditor-->>UnityConnection: SuccessResponse with prefabPath openedPrefabPath rootName enteredPrefabStage
end
end
UnityConnection-->>ServerManageEditorTool: Response payload
ServerManageEditorTool-->>LLMClient: Forward response
LLMClient-->>User: Report prefab editing state and details
Class diagram for updated ManageEditor prefab stage supportclassDiagram
class ManageEditor {
+static object HandleCommand(JObject params)
-static object OpenPrefabStage(string requestedPath)
-static object ClosePrefabStage()
}
class AssetPathUtility {
+static string SanitizeAssetPath(string requestedPath)
}
class ErrorResponse {
+ErrorResponse(string message)
}
class SuccessResponse {
+SuccessResponse(string message, object data)
}
class PrefabStageUtility {
+static PrefabStage GetCurrentPrefabStage()
}
class PrefabStage {
+string assetPath
+GameObject prefabContentsRoot
}
class AssetDatabase {
+static T LoadAssetAtPath~T~(string path)
+static bool OpenAsset(Object asset)
}
class GameObject {
+string name
}
ManageEditor ..> AssetPathUtility : uses
ManageEditor ..> ErrorResponse : returns
ManageEditor ..> SuccessResponse : returns
ManageEditor ..> PrefabStageUtility : queries
ManageEditor ..> PrefabStage : reads
ManageEditor ..> AssetDatabase : loads and opens assets
ManageEditor ..> GameObject : handles prefab assets
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an "open_prefab_stage" editor action and wiring to open a prefab in Unity's prefab-editing stage, including path validation and payload propagation; updates server tool surface, docs, and adds unit and Unity editor tests for the new workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant UnityEditor
Client->>Server: manage_editor(action="open_prefab_stage", prefab_path)
Server->>UnityEditor: send command {"action":"open_prefab_stage", "params":{"prefabPath": "..."}}
UnityEditor->>UnityEditor: validate & sanitize path
UnityEditor->>UnityEditor: load prefab asset, open PrefabStage
UnityEditor-->>Server: response { success, data:{prefabPath, openedPrefabPath, rootName, enteredPrefabStage} }
Server-->>Client: forward response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ManageEditor.cs" line_range="407" />
<code_context>
+ return new ErrorResponse($"Prefab asset not found at '{sanitizedPath}'.");
+ }
+
+ bool opened = AssetDatabase.OpenAsset(prefabAsset);
+ var prefabStage = PrefabStageUtility.GetCurrentPrefabStage();
+ bool enteredStage = prefabStage != null
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using prefab-stage-specific APIs instead of AssetDatabase.OpenAsset to guarantee entering prefab editing mode.
`AssetDatabase.OpenAsset(prefabAsset)` may only ping/select the asset or open it in an inspector depending on editor preferences, so `GetCurrentPrefabStage()` can still be null while `opened` is true. That means you may treat the operation as successful without actually entering prefab mode. To ensure the action truly opens the prefab stage, use the dedicated prefab stage APIs (e.g. `PrefabStageUtility.OpenPrefab`) and treat failure to enter a prefab stage as an error rather than relying on `OpenAsset`’s return value.
</issue_to_address>
### Comment 2
<location path="TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorPrefabStageTests.cs" line_range="98" />
<code_context>
+ }
+
+ [Test]
+ public void OpenPrefabStage_AcceptsPathAlias()
+ {
+ string prefabPath = CreateTestPrefab("AliasRoot");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test where both `prefabPath` and `path` are present in the request
Given `ManageEditor.HandleCommand` uses `prefabPath = p.Get("prefabPath") ?? p.Get("path")`, an extra test where both keys are present—and `prefabPath` is chosen—would explicitly verify and document that precedence behavior.
Suggested implementation:
```csharp
Assert.IsTrue(result.Value<bool>("success"));
```
Please add the following new test method to `ManageEditorPrefabStageTests` (ideally directly after `OpenPrefabStage_AcceptsPathAlias`), at class scope:
```csharp
[Test]
public void OpenPrefabStage_PrefabPathTakesPrecedenceOverPath()
{
string prefabPath = CreateTestPrefab("PrefabPathRoot");
string aliasPath = CreateTestPrefab("AliasPathRoot");
try
{
var result = ToJObject(ManageEditor.HandleCommand(new JObject
{
["action"] = "open_prefab_stage",
["prefabPath"] = prefabPath,
["path"] = aliasPath
}));
Assert.IsTrue(result.Value<bool>("success"));
var currentStage = PrefabStageUtility.GetCurrentPrefabStage();
Assert.IsNotNull(currentStage, "Expected a prefab stage to be open.");
Assert.AreEqual(
prefabPath,
currentStage.prefabAssetPath,
"prefabPath should take precedence over path when both are provided."
);
}
finally
{
StageUtility.GoToMainStage();
SafeDeleteAsset(prefabPath);
SafeDeleteAsset(aliasPath);
}
}
```
Notes/assumptions:
- This mirrors the pattern from the surrounding tests: `CreateTestPrefab(...)`, `try/finally` with `StageUtility.GoToMainStage()` and `SafeDeleteAsset(...)`.
- `prefabAssetPath` on `PrefabStageUtility.GetCurrentPrefabStage()` is used as in other tests (if they use `assetPath` or another property, adjust accordingly to match existing conventions).
- Ensure necessary `using` directives (e.g., `UnityEditor.SceneManagement`) are already present; they likely are given existing tests use `PrefabStageUtility` and `StageUtility`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| [Test] | ||
| public void OpenPrefabStage_AcceptsPathAlias() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test where both prefabPath and path are present in the request
Given ManageEditor.HandleCommand uses prefabPath = p.Get("prefabPath") ?? p.Get("path"), an extra test where both keys are present—and prefabPath is chosen—would explicitly verify and document that precedence behavior.
Suggested implementation:
Assert.IsTrue(result.Value<bool>("success"));Please add the following new test method to ManageEditorPrefabStageTests (ideally directly after OpenPrefabStage_AcceptsPathAlias), at class scope:
[Test]
public void OpenPrefabStage_PrefabPathTakesPrecedenceOverPath()
{
string prefabPath = CreateTestPrefab("PrefabPathRoot");
string aliasPath = CreateTestPrefab("AliasPathRoot");
try
{
var result = ToJObject(ManageEditor.HandleCommand(new JObject
{
["action"] = "open_prefab_stage",
["prefabPath"] = prefabPath,
["path"] = aliasPath
}));
Assert.IsTrue(result.Value<bool>("success"));
var currentStage = PrefabStageUtility.GetCurrentPrefabStage();
Assert.IsNotNull(currentStage, "Expected a prefab stage to be open.");
Assert.AreEqual(
prefabPath,
currentStage.prefabAssetPath,
"prefabPath should take precedence over path when both are provided."
);
}
finally
{
StageUtility.GoToMainStage();
SafeDeleteAsset(prefabPath);
SafeDeleteAsset(aliasPath);
}
}Notes/assumptions:
- This mirrors the pattern from the surrounding tests:
CreateTestPrefab(...),try/finallywithStageUtility.GoToMainStage()andSafeDeleteAsset(...). prefabAssetPathonPrefabStageUtility.GetCurrentPrefabStage()is used as in other tests (if they useassetPathor another property, adjust accordingly to match existing conventions).- Ensure necessary
usingdirectives (e.g.,UnityEditor.SceneManagement) are already present; they likely are given existing tests usePrefabStageUtilityandStageUtility.
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_editor.py (1)
44-52:⚠️ Potential issue | 🟡 MinorGuard against conflicting
prefab_pathandpathinputs.If both are provided with different values, the payload forwards both and downstream resolution is implicit. Add an explicit conflict check and send only one key to avoid ambiguous behavior.
Suggested fix
- params = { - "action": action, - "toolName": tool_name, - "tagName": tag_name, - "layerName": layer_name, - "prefabPath": prefab_path, - "path": path, - } + if prefab_path and path and prefab_path != path: + return { + "success": False, + "message": "Provide only one of prefab_path or path, or ensure both values match.", + } + + params = { + "action": action, + "toolName": tool_name, + "tagName": tag_name, + "layerName": layer_name, + } + if prefab_path is not None: + params["prefabPath"] = prefab_path + elif path is not None: + params["path"] = path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_editor.py` around lines 44 - 52, Detect when both prefab_path and path are provided with different values before building params in manage_editor.py: after the local variables (action, tool_name, tag_name, layer_name, prefab_path, path) are set but before creating the params dict, add an explicit conflict check that if prefab_path is not None and path is not None and prefab_path != path, then raise a ValueError (or a custom exception) explaining the conflict; otherwise proceed and include only the provided key (prefab_path or path) in the params dict so the subsequent params = {k: v for k, v in params.items() if v is not None} step does not send ambiguous/contradictory inputs.
🤖 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/ManageEditor.cs`:
- Around line 417-427: The current handler returns a SuccessResponse before
prefab editing is actually entered (enteredPrefabStage may be false); change the
method in ManageEditor.cs so it does not return success until the prefabStage is
fully entered by implementing an EditorApplication.update polling loop with a
TaskCompletionSource (same pattern as RefreshUnity.cs): start opening the
prefab, create a TaskCompletionSource<bool>, subscribe a callback to
EditorApplication.update that checks prefabStage/enteredPrefabStage (and
prefabStage.prefabContentsRoot) until true, then set the TCS result, unsubscribe
the update handler, and only then construct and return the SuccessResponse
(include prefabPath, openedPrefabPath, rootName, enteredPrefabStage) to ensure
callers only see success after entry is confirmed.
---
Outside diff comments:
In `@Server/src/services/tools/manage_editor.py`:
- Around line 44-52: Detect when both prefab_path and path are provided with
different values before building params in manage_editor.py: after the local
variables (action, tool_name, tag_name, layer_name, prefab_path, path) are set
but before creating the params dict, add an explicit conflict check that if
prefab_path is not None and path is not None and prefab_path != path, then raise
a ValueError (or a custom exception) explaining the conflict; otherwise proceed
and include only the provided key (prefab_path or path) in the params dict so
the subsequent params = {k: v for k, v in params.items() if v is not None} step
does not send ambiguous/contradictory inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80e98298-3755-4704-ad63-623491690196
📒 Files selected for processing (7)
MCPForUnity/Editor/Tools/ManageEditor.csServer/src/services/resources/prefab.pyServer/src/services/tools/manage_editor.pyServer/tests/test_manage_editor.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorPrefabStageTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorPrefabStageTests.cs.metaunity-mcp-skill/references/tools-reference.md
Comment on lines
+417
to
+427
| return new SuccessResponse( | ||
| enteredStage | ||
| ? $"Opened prefab stage for '{sanitizedPath}'." | ||
| : $"Requested prefab stage open for '{sanitizedPath}'. Unity may still be transitioning into prefab editing mode.", | ||
| new | ||
| { | ||
| prefabPath = sanitizedPath, | ||
| openedPrefabPath = enteredStage ? prefabStage.assetPath : sanitizedPath, | ||
| rootName = enteredStage && prefabStage.prefabContentsRoot != null ? prefabStage.prefabContentsRoot.name : prefabAsset.name, | ||
| enteredPrefabStage = enteredStage | ||
| } |
Contributor
There was a problem hiding this comment.
Do not return success before prefab stage entry is confirmed.
At Line 417, success is returned even when enteredPrefabStage is false (Line 420 message). That can break callers that issue immediate prefab-stage operations after a “successful” open.
Suggested fix
- return new SuccessResponse(
- enteredStage
- ? $"Opened prefab stage for '{sanitizedPath}'."
- : $"Requested prefab stage open for '{sanitizedPath}'. Unity may still be transitioning into prefab editing mode.",
- new
- {
- prefabPath = sanitizedPath,
- openedPrefabPath = enteredStage ? prefabStage.assetPath : sanitizedPath,
- rootName = enteredStage && prefabStage.prefabContentsRoot != null ? prefabStage.prefabContentsRoot.name : prefabAsset.name,
- enteredPrefabStage = enteredStage
- }
- );
+ if (!enteredStage)
+ {
+ return new ErrorResponse(
+ $"Failed to confirm prefab stage open for '{sanitizedPath}'."
+ );
+ }
+
+ return new SuccessResponse(
+ $"Opened prefab stage for '{sanitizedPath}'.",
+ new
+ {
+ prefabPath = sanitizedPath,
+ openedPrefabPath = prefabStage.assetPath,
+ rootName = prefabStage.prefabContentsRoot != null ? prefabStage.prefabContentsRoot.name : prefabAsset.name,
+ enteredPrefabStage = true
+ }
+ );As per coding guidelines, "C# async tool handlers must use EditorApplication.update polling with TaskCompletionSource pattern as shown in RefreshUnity.cs".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new SuccessResponse( | |
| enteredStage | |
| ? $"Opened prefab stage for '{sanitizedPath}'." | |
| : $"Requested prefab stage open for '{sanitizedPath}'. Unity may still be transitioning into prefab editing mode.", | |
| new | |
| { | |
| prefabPath = sanitizedPath, | |
| openedPrefabPath = enteredStage ? prefabStage.assetPath : sanitizedPath, | |
| rootName = enteredStage && prefabStage.prefabContentsRoot != null ? prefabStage.prefabContentsRoot.name : prefabAsset.name, | |
| enteredPrefabStage = enteredStage | |
| } | |
| if (!enteredStage) | |
| { | |
| return new ErrorResponse( | |
| $"Failed to confirm prefab stage open for '{sanitizedPath}'." | |
| ); | |
| } | |
| return new SuccessResponse( | |
| $"Opened prefab stage for '{sanitizedPath}'.", | |
| new | |
| { | |
| prefabPath = sanitizedPath, | |
| openedPrefabPath = prefabStage.assetPath, | |
| rootName = prefabStage.prefabContentsRoot != null ? prefabStage.prefabContentsRoot.name : prefabAsset.name, | |
| enteredPrefabStage = true | |
| } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageEditor.cs` around lines 417 - 427, The current
handler returns a SuccessResponse before prefab editing is actually entered
(enteredPrefabStage may be false); change the method in ManageEditor.cs so it
does not return success until the prefabStage is fully entered by implementing
an EditorApplication.update polling loop with a TaskCompletionSource (same
pattern as RefreshUnity.cs): start opening the prefab, create a
TaskCompletionSource<bool>, subscribe a callback to EditorApplication.update
that checks prefabStage/enteredPrefabStage (and prefabStage.prefabContentsRoot)
until true, then set the TCS result, unsubscribe the update handler, and only
then construct and return the SuccessResponse (include prefabPath,
openedPrefabPath, rootName, enteredPrefabStage) to ensure callers only see
success after entry is confirmed.
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
1 participant
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.
Summary
manage_editor(action="open_prefab_stage")to open a prefab asset in Prefab Stage.metafileValidation
cd Server && uv run pytest tests/test_manage_editor.py -qMCPForUnity/Editor/Tools/ManageEditor.cssuccessfullySummary by Sourcery
Add support for opening Unity prefab stages via the manage_editor tool and document its usage.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests