Replace command dispatcher with CommandRegistry, allow to add custom command handlers. #261
Conversation
WalkthroughCommand routing moved to a registry with snake_case command keys; Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant MCP as MCPForUnityBridge
participant Registry as CommandRegistry
participant Handler as Command Handler
rect rgb(230,240,255)
note right of Registry: Built-in handlers registered at startup\n(using snake_case keys)
Registry->>Registry: Register built-in handlers
end
rect rgb(235,255,235)
note over MCP,Registry: Runtime registration possible
MCP->>Registry: Add(commandName, handler)
Registry-->>MCP: registration stored
end
rect rgb(245,245,245)
note over MCP,Registry: Dispatch flow during ExecuteCommand
MCP->>Registry: GetHandler(command.type)
alt handler exists
Registry-->>MCP: Func<JObject, object>
MCP->>Handler: Invoke(paramsObject) [may use InvokeOnMainThreadWithTimeout]
Handler-->>MCP: result
MCP-->>Caller: wrapped success response
else unknown command
Registry-->>MCP: throws InvalidOperationException
MCP-->>Caller: wrapped error response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (2)
16-23: Command keys changed to snake_case — verify back-compat and casing; consider case-insensitive lookup and aliases.
- This is a breaking change if any caller still uses old names (e.g., "HandleManageAsset", "ManageAsset").
- Also, "manage_gameobject" is a potential naming outlier; confirm the intended key vs. "manage_game_object".
Suggested minimal hardening (case-insensitive lookups):
- private static readonly Dictionary<string, Func<JObject, object>> _handlers = new() + private static readonly Dictionary<string, Func<JObject, object>> _handlers = new(StringComparer.OrdinalIgnoreCase)Optionally, add aliases for a deprecation window:
+ { "HandleManageScript", ManageScript.HandleCommand }, + { "HandleManageScene", ManageScene.HandleCommand }, + { "HandleManageEditor", ManageEditor.HandleCommand }, + { "HandleManageGameObject", ManageGameObject.HandleCommand }, + { "HandleManageAsset", ManageAsset.HandleCommand }, + { "HandleReadConsole", ReadConsole.HandleCommand }, + { "HandleExecuteMenuItem", ExecuteMenuItem.HandleCommand }, + { "HandleManageShader", ManageShader.HandleCommand },Would you like me to scan this repo for any occurrences of the old command names to assess blast radius? I can provide a script.
29-40: XML docs out of sync with behavior; add TryGet variant or amend docs.
- Docs say: param example "HandleManageAsset" and “returns null otherwise”, but code now throws. Update docs or provide a non-throwing API.
Two-part fix:
- Update docs to reflect snake_case and throwing behavior.
- Provide a non-throwing helper for callers that prefer probing.
/// <param name="commandName">Name of the command handler (e.g., "HandleManageAsset").</param> -/// <returns>The command handler function if found, null otherwise.</returns> +/// <param name="commandName">Name of the command handler (e.g., "manage_asset").</param> +/// <returns>Always returns a handler if found.</returns> +/// <exception cref="InvalidOperationException">Thrown when the command name is not registered.</exception>+ public static bool TryGetHandler(string commandName, out Func<JObject, object> handler) + { + handler = null; + if (string.IsNullOrWhiteSpace(commandName)) return false; + return _handlers.TryGetValue(commandName, out handler); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (8)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
ManageScript(52-2546)HandleCommand(111-290)UnityMcpBridge/Editor/Tools/ManageAsset.cs (2)
HandleCommand(45-110)ManageAsset(25-1338)UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (2)
HandleCommand(28-62)ExecuteMenuItem(13-127)UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
HandleCommand(41-174)ManageGameObject(22-2167)UnityMcpBridge/Editor/Tools/ManageEditor.cs (2)
HandleCommand(28-146)ManageEditor(17-593)UnityMcpBridge/Editor/Tools/ManageShader.cs (2)
HandleCommand(20-123)ManageShader(15-341)UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
HandleCommand(22-112)ManageScene(17-425)UnityMcpBridge/Editor/Tools/ReadConsole.cs (3)
HandleCommand(129-202)ReadConsole(17-569)ReadConsole(37-125)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
33-37: Verify dynamic registration usage and thread-safety requirements
I didn’t find any calls toCommandRegistry.Addin the repo; please confirm whether commands are ever registered at runtime and if multi-threaded access or input-validation guards are actually needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (2)
457-458: Prevent editor freezes: use RunContinuationsAsynchronously for TCS.Without this,
tcs.SetResult(...)on the main thread can inline the awaiting continuation (await tcs.Task) and perform network I/O on the editor thread, stalling the UI under load.- TaskCompletionSource<string> tcs = new(); + TaskCompletionSource<string> tcs = + new(TaskCreationOptions.RunContinuationsAsynchronously);
345-355: Complete or cancel pending requests on Stop() to avoid hangs and leaks.If
Stop()is called while client handlers are awaitingtcs.Task, those awaits never complete becauseProcessCommandsis unsubscribed. This can leave connections stuck and tasks pinned.Apply within the try block after
isRunning = false;and before unsubscribingProcessCommands:try { // Mark as stopping early to avoid accept logging during disposal isRunning = false; + // Fail all pending requests so awaiting handlers can complete and exit + lock (lockObj) + { + foreach (var kvp in commandQueue.ToList()) + { + var id = kvp.Key; + var (_, tcs) = kvp.Value; + tcs.TrySetResult(JsonConvert.SerializeObject(new + { + status = "error", + error = "Bridge stopping", + command = "unknown" + })); + commandQueue.Remove(id); + } + } // Mark heartbeat one last time before stopping WriteHeartbeat(false, "stopped"); listener?.Stop(); listener = null; EditorApplication.update -= ProcessCommands;
🧹 Nitpick comments (5)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (5)
794-803: Registry dispatch looks good; verify external callers updated to snake_case.
CommandRegistry.GetHandler(command.type)now expects snake_case keys and throws for unknowns. Confirm clients (e.g., Python tooling) and docs have been migrated, or add aliases for backward compatibility if needed.If compatibility is required, consider a lightweight aliasing layer before lookup:
- object result = CommandRegistry.GetHandler(command.type)(paramsObject); + var commandName = command.type; + // Optional: alias legacy names to snake_case + commandName = LegacyCommandAliases.TryNormalize(commandName); + object result = CommandRegistry.GetHandler(commandName)(paramsObject);I can provide a small
LegacyCommandAliaseshelper if desired.
460-469: Deduplicate ping handling (three paths).
pingis handled in the client loop, inProcessCommands, and again inExecuteCommand. Keep only the fast path in the socket loop and remove the others to reduce branching and drift.Suggested approach:
- Keep the direct framed response in
HandleClientAsync.- Drop the special-cases in
ProcessCommandsandExecuteCommand.Also applies to: 669-681, 783-792
579-587: Minor robustness/readability in frame-length checks.Braces for the zero-length check and a slightly clearer message help diagnostics; functional behavior unchanged.
if (payloadLen > MaxFrameBytes) { throw new System.IO.IOException($"Invalid framed length: {payloadLen}"); } - if (payloadLen == 0UL) - throw new System.IO.IOException("Zero-length frames are not allowed"); + if (payloadLen == 0UL) + { + throw new System.IO.IOException("Zero-length frame"); + }
39-41: Make I/O timeouts configurable for troubleshooting.Hard-coding
FrameIOTimeoutMsat 30s can be restrictive in slow or CI environments. Allow an EditorPrefs override; default remains 30s.- private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients + // Per-read timeout to avoid stalled clients (override via EditorPrefs: "MCPForUnity.FrameTimeoutMs") + private static int FrameIOTimeoutMs => + Mathf.Max(1000, EditorPrefs.GetInt("MCPForUnity.FrameTimeoutMs", 30000));
635-736: Queue processing lacks backpressure; consider a soft cap.A misbehaving client can enqueue faster than
ProcessCommandsdrains, growingcommandQueue. Consider a soft cap (e.g., drop oldest or reject with 429-style error) to protect the editor.I can sketch a small ring-buffer or size-checked enqueue if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
CommandRegistry(10-46)
|
Thanks for contributing! We appreciate all PRs and effort the community puts into this repo. Looks good, haven't test it as yet to ensure nothing breaks. We don't run the CI tests every pull requests to mitigate costs (thinking we should change that until costs become a problem...), but I'm thinking this would break some of the unit tests in the TestProject. Can you run that test and let us know? It requires Unity 2021 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
43-46: Harden Add() against bad inputsRight now a plugin can register
CommandRegistry.Add("foo", null)and we’ll accept it, only to hit aNullReferenceExceptionwhen the command runs. Let’s fail fast by validating the inputs (non-empty name, non-null handler, and no accidental double registration).Here’s one way to guard the API:
- public static void Add(string commandName, Func<JObject, object> handler) - { - _handlers.Add(commandName, handler); - } + public static void Add(string commandName, Func<JObject, object> handler) + { + if (string.IsNullOrWhiteSpace(commandName)) + { + throw new ArgumentException("Command name must be provided.", nameof(commandName)); + } + + if (handler is null) + { + throw new ArgumentNullException(nameof(handler)); + } + + if (_handlers.ContainsKey(commandName)) + { + throw new InvalidOperationException($"A handler for command '{commandName}' is already registered."); + } + + _handlers.Add(commandName, handler); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (2)
UnityMcpBridge/Editor/Tools/ManageScript.cs (2)
ManageScript(52-2550)HandleCommand(111-290)UnityMcpBridge/Editor/Tools/ManageScene.cs (2)
HandleCommand(51-150)ManageScene(17-473)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
17-24: No internal PascalCase references; verify external producers emit snake_case
Ripgrep found no occurrences of the legacy PascalCase command names in this repo. Ensure any external bridges or scripts still emitting PascalCase tokens are updated to the new snake_case names to avoidInvalidOperationException.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs (1)
14-26: PreferAssert.Throwsfor exception assertionsNUnit’s fluent helpers make the intent clearer and remove the manual try/catch noise. Please switch to
Assert.Throws<InvalidOperationException>(() => CommandRegistry.GetHandler(unknown));.- try - { - var handler = CommandRegistry.GetHandler(unknown); - Assert.Fail("Should throw InvalidOperation for unknown handler."); - } - catch (InvalidOperationException) - { - - } - catch - { - Assert.Fail("Should throw InvalidOperation for unknown handler."); - } + Assert.Throws<InvalidOperationException>( + () => CommandRegistry.GetHandler(unknown), + "Should throw InvalidOperation for unknown handler.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs(1 hunks)
🔇 Additional comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs (1)
32-38: Nice verification of handler metadataThe extra checks on method name, declaring type, and static invocation tighten confidence that the registry is wiring the correct handler.
|
Will test and merge soon |
…command handlers. (CoplayDev#261) * Replace hard coded command dispatcher to command registry * Bug fixed. * bug fixed. * bug fixed. * Bug fixed. * Fix tests.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests