Files
integration/Plugins/BlueprintMCP/Source/BlueprintMCP/Private/Handlers/UMCPHandler_DeleteNodeFromGraph.Notes.md

2.0 KiB

UMCPHandler_DeleteNodeFromGraph - Refactoring Notes

What was done

  • Converted from JSON output (FJsonObject*) to plain-text output (FStringBuilderBase&).
  • MCPFetcher now receives Result (the FStringBuilderBase) as its error callback, so path resolution errors go directly to the caller as plain text.
  • Removed the three Result->SetStringField calls that echoed nodeClass/nodeTitle/graph back. The output now just confirms the deletion with a single line.
  • Removed both UE_LOG calls.
  • Error messages for protected entry nodes now go through MCPErrorCallback(Result) instead of MCPUtils::MakeErrorJson.
  • Removed the #include "EdGraph/EdGraph.h" include since EdGraphNode.h pulls it in (matching the example handler pattern).

What's good about this handler

  • The entry-node protection logic is solid and the error messages are detailed and helpful, explaining why the deletion is refused and what the user should do instead.
  • MCPFetcher handles all the object resolution cleanly — the Node path parameter does all the work.
  • The handler is concise; the bulk of the code is the entry-node guards, which is appropriate.

Uncertainties / conservative choices

  • I kept all three entry-node checks (UK2Node_FunctionEntry, UK2Node_Event, UK2Node_CustomEvent) as separate blocks with distinct error messages. They could be collapsed into one check, but the per-type messages are more informative.
  • UK2Node_CustomEvent inherits from UK2Node_Event, so the Cast<UK2Node_Event> check would catch custom events too. The current ordering (FunctionEntry, Event, CustomEvent) means the CustomEvent branch is unreachable. If the intent is to give a different message for custom events, the CustomEvent check should come before the Event check. I left the order as-is to minimize diff, but this is worth reviewing.
  • This handler operates on a single node. The coding standards mention batching where it makes sense. A batch-delete could be useful, but it changes the API signature, so I left it as single-node for now.