feat: integrate advanced UMG JSON and widget properties...#434
Open
Jbrandan wants to merge 6 commits into
Open
feat: integrate advanced UMG JSON and widget properties...#434Jbrandan wants to merge 6 commits into
Jbrandan wants to merge 6 commits into
Conversation
Contributor
|
👋 Thanks for your first Pull Request! We love contributions. Please ensure you have signed off your commits and followed the contribution guidelines. |
Contributor
📏 Large PR DetectedThis pull request is quite large (1000+ lines changed), which can make reviewing challenging. Suggestions:
This helps reviewers provide better feedback and speeds up the merge process. Thank you! 🙏 |
Contributor
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds JSON-first UMG tooling: property-name normalization, an attention-tracking editor subsystem, bidirectional UMG JSON export/import, read-only query and write subsystems, automation-bridge handlers, TypeScript tool/schema updates, and runtime dynamic handler loading. ChangesAdvanced UMG Widget Editing via JSON
Sequence DiagramsequenceDiagram
participant Client as TypeScript Client
participant Frontend as widget-authoring-handlers.ts
participant Bridge as McpAutomationBridge_WidgetAuthoringHandlers
participant FileXform as UUmgFileTransformation
participant Get as UUmgGetSubsystem
participant Set as UUmgSetSubsystem
participant Attention as UUmgAttentionSubsystem
Client->>Frontend: export_widget_tree action
Frontend->>Bridge: sendRequest(export_widget_tree)
Bridge->>FileXform: ExportUmgAssetToJsonString(assetPath, targetWidget)
FileXform-->>Bridge: widgetTreeJson
Bridge-->>Frontend: success + widgetTreeJson
Client->>Frontend: set_widget_properties action
Frontend->>Bridge: sendRequest(set_widget_properties, propertiesJson)
Bridge->>Set: SetWidgetProperties(blueprint, widgetName, propertiesJson)
Set->>FileXform: NormalizeJsonKeysToPascalCase(json)
FileXform-->>Set: normalized JSON
Set-->>Bridge: success
Bridge-->>Frontend: success
Client->>Frontend: get_layout_data action
Frontend->>Bridge: sendRequest(get_layout_data, resolution)
Bridge->>Get: GetLayoutData(blueprint, width, height)
Get-->>Bridge: layoutDataJson
Bridge-->>Frontend: success + layoutDataJson
Client->>Frontend: reparent_widget action
Frontend->>Bridge: sendRequest(reparent_widget, widgetName, newParent)
Bridge->>Set: ReparentWidget(blueprint, widgetName, newParent)
Set->>Attention: GetTargetUmgAsset() (for parent inference if needed)
Attention-->>Set: asset path
Set-->>Bridge: success
Bridge-->>Frontend: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NavigationHandlers.cpp (1)
136-296:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftResolve the checked-in merge conflict markers before merge.
<<<<<<<,=======, and>>>>>>>are still present across multiple handlers, so this file cannot compile and the final runtime behavior is ambiguous. Please resolve these regions explicitly and keep the safety-critical parts from the validated branch where the two sides disagree.Also applies to: 408-524, 598-673, 714-819, 910-1023, 1118-1177, 1241-1386, 1673-2105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NavigationHandlers.cpp` around lines 136 - 296, The file contains unresolved Git merge conflict markers (<<<<<<<, =======, >>>>>>>) that prevent compilation; locate each conflicted handler such as HandleConfigureNavMeshSettings and other navigation handlers (references: UMcpAutomationBridgeSubsystem, HandleConfigureNavMeshSettings, SendAutomationResponse, ARecastNavMesh usage) and remove the conflict markers by explicitly choosing and merging the correct code paths (prefer the validated/safety-first branch where they differ), ensuring you preserve validation checks (blueprintPath checks, World/NavSys/NavMesh null handling), conditional engine-version blocks, and the final SendAutomationResponse(Result) behavior; after resolving each region also run a build to confirm no leftover markers or mismatched braces remain.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp (1)
1128-1158:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the extra
});aftercheck_pie_state.Line 1158 adds a second close for the same
RegisterHandler(...)call, which leaves this translation unit uncompilable.🛠️ Proposed fix
RegisterHandler(TEXT("check_pie_state"), [this](const FString &R, const FString &A, const TSharedPtr<FJsonObject> &P, TSharedPtr<FMcpBridgeWebSocket> S) { `#if` WITH_EDITOR ... `#else` ... `#endif` }); - }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp` around lines 1128 - 1158, The extra closing sequence "});" after the RegisterHandler(TEXT("check_pie_state"), ...) block should be removed to fix the unmatched braces; locate the RegisterHandler call for "check_pie_state" in McpAutomationBridgeSubsystem.cpp and delete the redundant trailing "});" so the lambda closing, the RegisterHandler call, and surrounding scope close correctly while leaving the existing SendAutomationResponse and SendAutomationError usage intact.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_VolumeHandlers.cpp (2)
46-65:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the merge-conflict markers before merging.
This file still contains
<<<<<<<,=======, and>>>>>>>blocks, so it will not compile. The same pattern appears again later in the file, so the whole translation unit needs a clean merge before anything else.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_VolumeHandlers.cpp` around lines 46 - 65, The file contains unresolved Git merge conflict markers (<<<<<<<, =======, >>>>>>>) around the includes; remove the conflict block and perform a proper merge so both needed headers and the post-process version check are preserved (or intentionally chosen) without duplication. Specifically, reconcile/include the relevant headers such as CameraBlockingVolume, BrushComponent, SphereComponent, CapsuleComponent, Brush, Polys, CubeBuilder, PostProcessVolume with the ENGINE_MAJOR/MINOR check and MCP_HAS_POSTPROCESS_VOLUME macro, and also include Sound/AudioVolume and Sound/ReverbEffect if required; ensure the MCP_HAS_POSTPROCESS_VOLUME logic and symbol names (e.g., MCP_HAS_POSTPROCESS_VOLUME, PostProcessVolume, AudioVolume, ReverbEffect) remain consistent and there are no leftover conflict markers anywhere in McpAutomationBridge_VolumeHandlers.cpp.
82-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the input-validation helpers when resolving this branch.
The stashed side drops
ValidateVolumeName,ValidateExtent,ValidateRadius,ValidateCapsuleDimensions, andValidateLocation. If that version wins, the create/update handlers below will accept NaN/Inf/negative sizes and unchecked labels straight into brush construction, actor scaling, and collision sizing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_VolumeHandlers.cpp` around lines 82 - 383, The merge resolution must preserve the input-validation helper functions (ValidateVolumeName, ValidateExtent, ValidateRadius, ValidateCapsuleDimensions, ValidateLocation) and the editor-only utility functions (CreateBoxBrushForVolume, CreateSphereBrushForVolume, CreateCapsuleBrushForVolume, GetVectorFromPayload, GetRotatorFromPayload, GetEditorWorld, SpawnVolumeActor templates) from the updated-upstream side instead of dropping them to the stashed side; restore those validation helpers inside the WITH_EDITOR block and ensure handlers that call them (e.g., CreateBoxBrushForVolume and the SpawnVolumeActor overloads) still reference the validators so NaN/Inf/negative sizes and invalid volume names are rejected during create/update.
🧹 Nitpick comments (3)
src/tools/consolidated-tool-definitions.ts (1)
3192-3200: 🏗️ Heavy liftAvoid modeling the new widget tree/property payloads as opaque strings.
These fields are the core contract for the feature, but
type: 'string'means any malformed blob satisfies the schema and output validation can’t verify the returned structure either. Since tool calls are already JSON, it would be better to define these as object/array schemas here and stringify/parse only at the bridge boundary if C++ still needs strings. As per coding guidelines, “Tool schemas and action enums live in src/tools/consolidated-tool-definitions.ts. Treat that file as the source of truth for inputs and outputs” and “Output schemas are registered during startup and validated before responses leave the MCP server.”Also applies to: 3514-3517
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/consolidated-tool-definitions.ts` around lines 3192 - 3200, The schema currently models payloads like widgetTreeJson and propertiesJson (and similarly the fields around lines 3514–3517) as type: 'string', which prevents structural validation; change those entries in consolidated-tool-definitions.ts to proper JSON schemas (e.g., widgetTreeJson: { type: 'object', properties: { ... } } or an array schema for properties) matching the expected structure instead of opaque strings, update properties to be an array schema with item type definitions, and ensure any stringification/parsing happens only at the bridge boundary (C++ interop) so the MCP startup registration can validate inputs/outputs against these new object/array schemas.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/PropertyNameMappings.h (1)
19-88: ⚡ Quick winDerive the reverse map from the forward map instead of maintaining two copies.
These tables encode the same data twice. The next added key only needs to be missed once for import/export behavior to become asymmetric.
Suggested direction
static const TMap<FString, FString>& GetReversePropertyNameMappings() { - static TMap<FString, FString> ReverseMappings = { - {"SizeRule", "sizeRule"}, - {"Value", "value"}, - {"HorizontalAlignment", "horizontalAlignment"}, - {"VerticalAlignment", "verticalAlignment"}, - {"Padding", "padding"}, - {"Left", "left"}, - {"Top", "top"}, - {"Right", "right"}, - {"Bottom", "bottom"}, - {"R", "r"}, - {"G", "g"}, - {"B", "b"}, - {"A", "a"}, - {"Size", "size"}, - {"TypefaceFontName", "typefaceFontName"}, - {"IsEnabled", "isEnabled"}, - {"Visibility", "visibility"}, - {"RenderOpacity", "renderOpacity"}, - {"ToolTipText", "toolTipText"}, - }; + static TMap<FString, FString> ReverseMappings = []() + { + TMap<FString, FString> Result; + for (const TPair<FString, FString>& Entry : GetPropertyNameMappings()) + { + Result.Add(Entry.Value, Entry.Key); + } + return Result; + }(); return ReverseMappings; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/PropertyNameMappings.h` around lines 19 - 88, Get rid of the duplicated hard-coded reverse table by building ReverseMappings programmatically from GetPropertyNameMappings(): in GetReversePropertyNameMappings(), obtain the forward map via GetPropertyNameMappings(), iterate its entries and insert inverted pairs into a static TMap<FString,FString> (or initialize the static with a lambda that constructs the inverted map), making sure to handle/overwrite collisions deterministically (e.g., last-wins or log) so import/export stays symmetric; keep GetPropertyNameMappings() unchanged and return the constructed ReverseMappings.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cpp (1)
61-64: ⚡ Quick winCap
UmgAssetHistoryto the advertised recent-window size.Both insertion sites grow the history forever, while the public API describes a bounded recent-history view. Trimming here keeps the stored state aligned with that contract.
Suggested fix
UmgAssetHistory.Remove(AssetPath); UmgAssetHistory.Insert(AssetPath, 0); + constexpr int32 MaxTrackedAssets = 5; + if (UmgAssetHistory.Num() > MaxTrackedAssets) + { + UmgAssetHistory.SetNum(MaxTrackedAssets); + }Also applies to: 165-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cpp` around lines 61 - 64, After inserting AssetPath into UmgAssetHistory (via UmgAssetHistory.Remove/Insert), trim UmgAssetHistory to the configured recent-window size used by the public API: after each UmgAssetHistory.Insert(AssetPath, 0) check if UmgAssetHistory.Num() > RecentWindow (or the actual recent-window config variable used elsewhere) and remove oldest entries until Num() == RecentWindow; apply the same trimming logic at the other insertion site as well (the second UmgAssetHistory.Insert occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SessionsHandlers.cpp`:
- Around line 741-807: The OnlineSubsystem fallback always builds a net ID from
TargetPlayerId so playerName is ignored; update the net-ID resolution to use
TargetIdentifier (which is set from TargetPlayerId or PlayerName) instead of
TargetPlayerId when calling IdentityInterface->CreateUniquePlayerId (or
equivalent GetUniquePlayerId/lookup for UE5.7+), and ensure the resolved
FUniqueNetIdPtr (or local-player unique id) is then passed to
VoiceInterface->MuteRemoteTalker/UnmuteRemoteTalker; reference TargetIdentifier,
TargetPlayerId, PlayerName, IdentityInterface->CreateUniquePlayerId, and
VoiceInterface->MuteRemoteTalker/UnmuteRemoteTalker to locate and change the
code paths.
- Around line 166-175: The handler currently returns false after detecting an
invalid InterfaceType, which signals the request was unhandled; change it to
return true after calling Subsystem->SendAutomationResponse so the caller knows
the error was already handled. Locate the check using ValidTypes and
InterfaceType in McpAutomationBridge_SessionsHandlers.cpp and update the return
value after the Subsystem->SendAutomationResponse(...) call (reference
Subsystem->SendAutomationResponse, InterfaceType, ValidTypes) to return true
instead of false.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`:
- Around line 6173-6174: The code dereferences GEditor and the editor subsystem
without null checks (lines using GEditor->GetEditorSubsystem<UUmgGetSubsystem>()
and calling QueryWidgetProperties), which can crash when the editor or subsystem
is unavailable; update each occurrence (e.g., the calls around GetSubsystem,
QueryWidgetProperties, and similar blocks at the other noted locations) to
guard: check GEditor != nullptr, call
GEditor->GetEditorSubsystem<UUmgGetSubsystem>() and verify the returned
UUmgGetSubsystem* is non-null before calling QueryWidgetProperties(WidgetBP,
WidgetName, Properties); if any check fails, return a structured automation
error/result instead of proceeding to dereference.
- Around line 6244-6276: There’s a duplicate and unreachable handling of
SubAction.Equals(TEXT("reparent_widget")) — fix by consolidating the logic so
only one branch handles the action and it accepts the new parameter names;
either remove this later block or merge its behavior into the existing
reparent_widget handler so that GetJsonStringField calls accept both
"slotName"/"newParent" and "widgetName"/"newParentName" (or normalize incoming
keys before validation), then call LoadWidgetBlueprint,
UUmgSetSubsystem->ReparentWidget(WidgetBP, WidgetName, NewParentName) and use
SendAutomationResponse/SendAutomationError and ResultJson->SetBoolField exactly
once.
- Around line 6104-6111: The handler is passing raw WidgetPath into
UUmgFileTransformation::ExportUmgAssetToJsonString (and similar calls to
LoadWidgetBlueprint) without sanitization; call SanitizeProjectRelativePath() on
the incoming WidgetPath, verify the sanitized path is non-empty and safe (reject
traversal/absolute paths), send an appropriate SendAutomationError (e.g.,
"Invalid widgetPath" / "MISSING_PARAMETER" or a new "INVALID_PATH" code) when
sanitization fails, and then use the sanitized path variable when invoking
UUmgFileTransformation::ExportUmgAssetToJsonString and LoadWidgetBlueprint;
apply this same sanitization guard to every handler that reads widgetPath before
dispatching ExportUmgAssetToJsonString or LoadWidgetBlueprint.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cpp`:
- Line 2: The current self-header include "FileManage/UmgAttentionSubsystem.h"
is using the wrong path and causes a compile error; update the include in
UmgAttentionSubsystem.cpp to reference the public header directly (e.g., include
"UmgAttentionSubsystem.h") so it matches the module's exported Public/ include
root and resolves the header for the UmgAttentionSubsystem implementation.
- Around line 156-163: When switching to a new TargetBP in SetAttentionTarget
(the block that sets AttentionTargetAssetPath and CachedTargetWidgetBlueprint),
also reset the remaining attention context fields to avoid stale state: clear
CurrentWidgetName (already done) and likewise clear CurrentAnimationName and
CurrentGraphName, reset LastEditedNodeId to an invalid/default value, and reset
CurrentNodePosition to its default/zero value; make these resets in the same
TargetBP success branch (near where AttentionTargetAssetPath and
CachedTargetWidgetBlueprint are assigned) so all per-asset state is cleared when
the asset changes.
- Around line 119-154: The SetTargetUmgAsset implementation currently creates a
new UWidgetBlueprint when AssetPath is valid but missing (using
AssetTools/CreateAsset and UWidgetBlueprintFactory), which must be removed;
instead, stop performing any asset-creation side effects in SetTargetUmgAsset —
remove or disable the block that loads AssetTools, constructs
UWidgetBlueprintFactory, and calls CreateAsset, and replace it with a
non-mutating behavior (e.g., log a warning and leave TargetBP null or return
early). Keep checks for FPackageName::IsValidPath(AssetPath) and use
AssetPath/TargetBP to produce informative logs, but do not call NewObject,
CreateAsset, or cast to UWidgetBlueprint in this function. Ensure no write
operations or module asset creation occur from SetTargetUmgAsset.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cpp`:
- Around line 282-295: The function currently queues
ApplyJsonToUmgAsset_GameThread and returns true immediately, producing false
positives; instead change ApplyJsonToUmgAsset_GameThread to return a bool
success value and make ApplyJsonStringToUmgAsset create a
TPromise<bool>/TFuture<bool>, capture the promise in the lambda passed to
FFunctionGraphTask, call ApplyJsonToUmgAsset_GameThread inside that lambda and
SetValue on the promise with the returned bool, then have
ApplyJsonStringToUmgAsset wait on the future (or Get with a reasonable timeout)
and return the actual result so callers (e.g.
McpAutomationBridge_WidgetAuthoringHandlers) only report success after the
game-thread work completes.
- Line 2: The include path is wrong in UmgFileTransformation.cpp: replace the
incorrect "`#include` \"FileManage/UmgFileTransformation.h\"" with the same
include used by UmgSetSubsystem.cpp / UmgGetSubsystem.cpp so it references the
public header directly (UmgFileTransformation.h) to match the actual header
location and resolve the compile error.
- Around line 662-676: CreateWidgetFromJson currently only handles UPanelWidget
parents; add a branch to also support UContentWidget parents by casting
ParentWidget to UContentWidget and calling SetContent(NewWidget) (use
ParentContent->SetContent(NewWidget)). After setting content, retrieve the
resulting slot (so slot properties can be applied later) — e.g., obtain the
content slot from the UContentWidget or from NewWidget so you can assign NewSlot
similarly to the UPanelWidget path — and log a warning if SetContent or slot
retrieval fails; keep using existing variables NewWidget, NewSlot, ParentWidget,
ParentName, WidgetName and preserve the existing UE_LOG patterns.
- Around line 520-537: The existing-widget path in ApplyJsonStringToUmgAsset
applies WidgetProps/SlotProps with FJsonObjectConverter::JsonObjectToUStruct but
does not resolve object-path strings into actual UObject references (e.g.,
FSlateBrush.ResourceObject or generic FObjectProperty), so object-backed
properties are dropped; replicate the resolution performed by UUm
gSetSubsystem::SetWidgetProperties by detecting object-path string values in
WidgetProps and NormalizedSlotProps, calling LoadObject (or the same resolver
used by SetWidgetProperties) and assigning the resolved UObject to the
corresponding FObjectProperty/FStructProperty (e.g., FSlateBrush.ResourceObject)
on TargetWidget and TargetWidget->Slot before or after JsonObjectToUStruct,
ensuring you handle both direct FObjectProperty fields and nested struct fields
like FSlateBrush.ResourceObject and use
UUmgFileTransformation::NormalizeJsonKeysToPascalCase for SlotProps as currently
done.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cpp`:
- Around line 408-430: The code currently checks every widget in WidgetBlueprint
(AllWidgets) and returns true if any two overlap, ignoring the requested
WidgetIds; limit the overlap check to only the widgets in WidgetIds by filtering
when building BoundingBoxes: for each UWidget in AllWidgets, only call
GetCachedWidget()/GetTickSpaceGeometry().GetLayoutBoundingRect() and add to
BoundingBoxes if the widget's identifier matches an entry in WidgetIds (compare
Widget->GetFName() or Widget->GetName() to the values in WidgetIds depending on
WidgetIds' type), then perform the pairwise FSlateRect::DoRectanglesIntersect
loop on that filtered BoundingBoxes set (symbols: WidgetIds, AllWidgets,
BoundingBoxes, WidgetBlueprint, GetCachedWidget, GetTickSpaceGeometry,
GetLayoutBoundingRect).
- Around line 350-391: GetLayoutData currently serializes the cached widget
bounding rects but ignores the ResolutionWidth/ResolutionHeight parameters;
update GetLayoutData to scale the serialized coordinates to the requested
resolution by computing scaleX and scaleY from the current viewport (e.g.,
obtain current viewport size via GEngine->GameViewport->Viewport->GetSizeXY())
and then multiply BoundingBox.Left/Top/Right/Bottom by scaleX/scaleY before
adding them to WidgetLayoutJson; keep this logic inside
UUmgGetSubsystem::GetLayoutData so the caller-provided
ResolutionWidth/ResolutionHeight affect the returned JSON.
- Line 2: Update the broken include to reference the public header by replacing
the current include with the existing public header "UmgGetSubsystem.h", and
modify the implementations of GetLayoutData and CheckWidgetOverlap so they
actually use the API parameters: in GetLayoutData use ResolutionWidth and
ResolutionHeight to compute/layout bounds or scale coordinates before returning
layout info (ensure you apply the provided resolution when computing widget
positions/sizes), and in CheckWidgetOverlap filter the set of widgets by the
provided WidgetIds parameter (only test overlap among those IDs, not every
widget in the blueprint) and return overlap results scoped to those IDs; locate
these changes in the functions named GetLayoutData(...) and
CheckWidgetOverlap(...) in UmgGetSubsystem.cpp and adjust logic to respect
ResolutionWidth, ResolutionHeight, and WidgetIds accordingly.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp`:
- Line 3: The include in UmgSetSubsystem.cpp is pointing to a non-existent path
("Widget/UmgSetSubsystem.h"); update the include to reference the actual header
name used in the plugin by replacing the include with "UmgSetSubsystem.h" so the
compiler can find the UmgSetSubsystem declaration (search for the `#include` line
in UmgSetSubsystem.cpp and change it to include "UmgSetSubsystem.h").
- Around line 533-543: Replace the modal checkout/save call in
UUmgSetSubsystem::SaveAsset (currently using
FEditorFileUtils::PromptForCheckoutAndSave with PackagesToSave) with the
non-modal helper McpSafeAssetSave(WidgetBlueprint) from
McpAutomationBridgeHelpers.h; call McpSafeAssetSave and branch on its boolean
result to log success or error with WidgetBlueprint->GetPathName() and return
true/false accordingly, removing any UI-prompt usage
(FEditorFileUtils::PromptForCheckoutAndSave) to keep automation safe and
non-interactive.
- Around line 381-405: The CreateWidget flow currently only accepts UPanelWidget
for root/parent; update UUmgSetSubsystem::CreateWidget to allow non-panel roots
and parents by treating any widget class as a valid RootWidget (remove the
strict IsChildOf(UPanelWidget) restriction) and, when attaching children, check
the found parent widget: if Cast<UPanelWidget>(Parent) use
UPanelWidget::AddChild, else if Cast<UContentWidget>(Parent) use
UContentWidget::SetContent, otherwise log a clear error and return. Use the
existing symbols WidgetClass, bCreatingRootWidget,
WidgetBlueprint->WidgetTree->RootWidget, ActualParentName, ParentWidget and call
AddChild/SetContent accordingly so content-style widgets
(Border/Button/ScaleBox/SizeBox) can be roots or parents.
In `@src/tools/handlers/widget-authoring-handlers.ts`:
- Around line 569-607: The handler cases (export_widget_tree, apply_widget_tree,
query_widget_properties, set_widget_properties, get_layout_data,
reparent_widget, delete_widget) only call requireNonEmptyString and forward to
sendRequest, so invalid JSON and unnormalized widgetPath values get sent; update
each case to sanitize and normalize argsRecord.widgetPath via sanitizePath (or
the domain normalizer) before validation/forwarding, parse and validate JSON
payloads widgetTreeJson and propertiesJson (throw/return a validation error if
JSON.parse fails or the payload schema is invalid) and then call
executeAutomationRequest/sendRequest with the sanitized argsRecord; ensure
references to requireNonEmptyString, sanitizePath, widgetTreeJson,
propertiesJson, sendRequest, and executeAutomationRequest are used so the
handler rejects malformed JSON and normalized paths at the boundary.
---
Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NavigationHandlers.cpp`:
- Around line 136-296: The file contains unresolved Git merge conflict markers
(<<<<<<<, =======, >>>>>>>) that prevent compilation; locate each conflicted
handler such as HandleConfigureNavMeshSettings and other navigation handlers
(references: UMcpAutomationBridgeSubsystem, HandleConfigureNavMeshSettings,
SendAutomationResponse, ARecastNavMesh usage) and remove the conflict markers by
explicitly choosing and merging the correct code paths (prefer the
validated/safety-first branch where they differ), ensuring you preserve
validation checks (blueprintPath checks, World/NavSys/NavMesh null handling),
conditional engine-version blocks, and the final SendAutomationResponse(Result)
behavior; after resolving each region also run a build to confirm no leftover
markers or mismatched braces remain.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_VolumeHandlers.cpp`:
- Around line 46-65: The file contains unresolved Git merge conflict markers
(<<<<<<<, =======, >>>>>>>) around the includes; remove the conflict block and
perform a proper merge so both needed headers and the post-process version check
are preserved (or intentionally chosen) without duplication. Specifically,
reconcile/include the relevant headers such as CameraBlockingVolume,
BrushComponent, SphereComponent, CapsuleComponent, Brush, Polys, CubeBuilder,
PostProcessVolume with the ENGINE_MAJOR/MINOR check and
MCP_HAS_POSTPROCESS_VOLUME macro, and also include Sound/AudioVolume and
Sound/ReverbEffect if required; ensure the MCP_HAS_POSTPROCESS_VOLUME logic and
symbol names (e.g., MCP_HAS_POSTPROCESS_VOLUME, PostProcessVolume, AudioVolume,
ReverbEffect) remain consistent and there are no leftover conflict markers
anywhere in McpAutomationBridge_VolumeHandlers.cpp.
- Around line 82-383: The merge resolution must preserve the input-validation
helper functions (ValidateVolumeName, ValidateExtent, ValidateRadius,
ValidateCapsuleDimensions, ValidateLocation) and the editor-only utility
functions (CreateBoxBrushForVolume, CreateSphereBrushForVolume,
CreateCapsuleBrushForVolume, GetVectorFromPayload, GetRotatorFromPayload,
GetEditorWorld, SpawnVolumeActor templates) from the updated-upstream side
instead of dropping them to the stashed side; restore those validation helpers
inside the WITH_EDITOR block and ensure handlers that call them (e.g.,
CreateBoxBrushForVolume and the SpawnVolumeActor overloads) still reference the
validators so NaN/Inf/negative sizes and invalid volume names are rejected
during create/update.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp`:
- Around line 1128-1158: The extra closing sequence "});" after the
RegisterHandler(TEXT("check_pie_state"), ...) block should be removed to fix the
unmatched braces; locate the RegisterHandler call for "check_pie_state" in
McpAutomationBridgeSubsystem.cpp and delete the redundant trailing "});" so the
lambda closing, the RegisterHandler call, and surrounding scope close correctly
while leaving the existing SendAutomationResponse and SendAutomationError usage
intact.
---
Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/PropertyNameMappings.h`:
- Around line 19-88: Get rid of the duplicated hard-coded reverse table by
building ReverseMappings programmatically from GetPropertyNameMappings(): in
GetReversePropertyNameMappings(), obtain the forward map via
GetPropertyNameMappings(), iterate its entries and insert inverted pairs into a
static TMap<FString,FString> (or initialize the static with a lambda that
constructs the inverted map), making sure to handle/overwrite collisions
deterministically (e.g., last-wins or log) so import/export stays symmetric;
keep GetPropertyNameMappings() unchanged and return the constructed
ReverseMappings.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cpp`:
- Around line 61-64: After inserting AssetPath into UmgAssetHistory (via
UmgAssetHistory.Remove/Insert), trim UmgAssetHistory to the configured
recent-window size used by the public API: after each
UmgAssetHistory.Insert(AssetPath, 0) check if UmgAssetHistory.Num() >
RecentWindow (or the actual recent-window config variable used elsewhere) and
remove oldest entries until Num() == RecentWindow; apply the same trimming logic
at the other insertion site as well (the second UmgAssetHistory.Insert
occurrence).
In `@src/tools/consolidated-tool-definitions.ts`:
- Around line 3192-3200: The schema currently models payloads like
widgetTreeJson and propertiesJson (and similarly the fields around lines
3514–3517) as type: 'string', which prevents structural validation; change those
entries in consolidated-tool-definitions.ts to proper JSON schemas (e.g.,
widgetTreeJson: { type: 'object', properties: { ... } } or an array schema for
properties) matching the expected structure instead of opaque strings, update
properties to be an array schema with item type definitions, and ensure any
stringification/parsing happens only at the bridge boundary (C++ interop) so the
MCP startup registration can validate inputs/outputs against these new
object/array schemas.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e760e05-c817-4a7e-9691-93ff5cc3132d
📒 Files selected for processing (18)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_LevelStructureHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_NavigationHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SessionsHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_SplineHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_VolumeHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/PropertyNameMappings.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgAttentionSubsystem.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgFileTransformation.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgGetSubsystem.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgSetSubsystem.hsrc/tools/consolidated-tool-definitions.tssrc/tools/handlers/widget-authoring-handlers.ts
Comment on lines
+381
to
+405
| if (!WidgetBlueprint->WidgetTree->RootWidget) | ||
| { | ||
| // If the tree is empty, ANY creation request must be for the root. | ||
| // We are lenient here: if the widget type is a Panel (valid for root), we allow it regardless of what ParentName the AI sent. | ||
|
|
||
| if (WidgetClass->IsChildOf(UPanelWidget::StaticClass())) | ||
| { | ||
| bCreatingRootWidget = true; | ||
| UE_LOG(LogUmgSet, Log, TEXT("CreateWidget: No root widget exists. Auto-promoting '%s' to root widget."), *WidgetType); | ||
| } | ||
| else | ||
| { | ||
| UE_LOG(LogUmgSet, Error, TEXT("CreateWidget: Cannot create '%s' as root widget. Root widget must be a Panel type (e.g., VerticalBox, HorizontalBox, CanvasPanel)."), *WidgetType); | ||
| return FString(); | ||
| } | ||
| } | ||
|
|
||
| if (!bCreatingRootWidget) | ||
| { | ||
| // Normal case: creating a child widget, need to find parent | ||
| ParentWidget = Cast<UPanelWidget>(WidgetBlueprint->WidgetTree->FindWidget(FName(*ActualParentName))); | ||
| if (!ParentWidget) | ||
| { | ||
| UE_LOG(LogUmgSet, Error, TEXT("CreateWidget: Failed to find ParentWidget with name '%s' in asset '%s'."), *ActualParentName, *WidgetBlueprint->GetPathName()); | ||
| return FString(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp"
echo "== File stats =="
wc -l "$FILE" || true
echo
echo "== Lines 340-460 (context around 381-405) =="
nl -ba "$FILE" | sed -n '340,460p'
echo
echo "== Search for WidgetTree->RootWidget / FindWidget / parent lookup usage =="
rg -n "WidgetTree->RootWidget|FindWidget\\(|ActualParentName|bCreatingRootWidget|UPanelWidget::StaticClass|Cast<UPanelWidget>" "$FILE"
echo
echo "== Search for UMG widget root/parent creation logic in nearby files =="
rg -n "UmgSetSubsystem|CreateWidget:|Auto-promoting|Failed to find ParentWidget" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private
echo
echo "== Search for helpers affecting UMG widget creation =="
rg -n "McpSafeAssetSave|McpSafeLevelSave|McpSafeLoadMap|McpAutomationBridgeHelpers|UMG|WidgetBlueprint|WidgetTree" plugins/McpAutomationBridge/Source/McpAutomationBridgeRepository: ChiR24/Unreal_mcp
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -uo pipefail
FILE="plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp"
echo "== File stats =="
wc -l "$FILE" || true
echo
echo "== Lines 340-460 (context around 381-405) =="
awk 'NR>=340 && NR<=460 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Search for WidgetTree->RootWidget / FindWidget / parent lookup usage =="
rg -n "WidgetTree->RootWidget|FindWidget\\(|ActualParentName|bCreatingRootWidget|UPanelWidget::StaticClass|Cast<UPanelWidget>" "$FILE" || true
echo
echo "== Search for UMG widget root/parent creation logic in nearby files =="
rg -n "UmgSetSubsystem|CreateWidget:|Auto-promoting|Failed to find ParentWidget" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private || true
echo
echo "== Search for helpers affecting UMG widget creation =="
rg -n "McpSafeAssetSave|McpSafeLevelSave|McpSafeLoadMap|McpAutomationBridgeHelpers|UMG|WidgetBlueprint|WidgetTree" plugins/McpAutomationBridge/Source/McpAutomationBridge || trueRepository: ChiR24/Unreal_mcp
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -uo pipefail
FILE="plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp"
echo "== File stats =="
wc -l "$FILE" || true
echo
echo "== Lines 340-460 (context around 381-405) =="
awk 'NR>=340 && NR<=460 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Search for WidgetTree->RootWidget / FindWidget / parent lookup usage =="
rg -n "WidgetTree->RootWidget|FindWidget\\(|ActualParentName|bCreatingRootWidget|UPanelWidget::StaticClass|Cast<UPanelWidget>" "$FILE" || true
echo
echo "== Search for UMG widget root/parent creation logic in nearby files =="
rg -n "UmgSetSubsystem|CreateWidget:|Auto-promoting|Failed to find ParentWidget" plugins/McpAutomationBridge/Source/McpAutomationBridge/Private || true
echo
echo "== Search for helpers affecting UMG widget creation =="
rg -n "McpSafeAssetSave|McpSafeLevelSave|McpSafeLoadMap|McpAutomationBridgeHelpers|UMG|WidgetBlueprint|WidgetTree" plugins/McpAutomationBridge/Source/McpAutomationBridge || trueRepository: ChiR24/Unreal_mcp
Length of output: 50374
UMG CreateWidget incorrectly restricts root/parent widgets to UPanelWidget
UUmgSetSubsystem::CreateWidget only allows:
- Root creation (when
WidgetTree->RootWidgetis missing) ifWidgetClassis aUPanelWidgetsubclass; otherwise it errors and returns. - Child parenting if the resolved parent widget name casts to
UPanelWidget; it then callsUPanelWidget::AddChild.
This prevents using non-UPanelWidget containers (e.g., Border/Button/ScaleBox/SizeBox and other content-style widgets) as roots or parents through this API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp`
around lines 381 - 405, The CreateWidget flow currently only accepts
UPanelWidget for root/parent; update UUmgSetSubsystem::CreateWidget to allow
non-panel roots and parents by treating any widget class as a valid RootWidget
(remove the strict IsChildOf(UPanelWidget) restriction) and, when attaching
children, check the found parent widget: if Cast<UPanelWidget>(Parent) use
UPanelWidget::AddChild, else if Cast<UContentWidget>(Parent) use
UContentWidget::SetContent, otherwise log a clear error and return. Use the
existing symbols WidgetClass, bCreatingRootWidget,
WidgetBlueprint->WidgetTree->RootWidget, ActualParentName, ParentWidget and call
AddChild/SetContent accordingly so content-style widgets
(Border/Button/ScaleBox/SizeBox) can be roots or parents.
Jbrandan
force-pushed
the
feat/advanced-umg-control
branch
from
bd34787 to
20da112
Compare
Owner
|
Thanks for putting this together. I noticed a few things that may need a quick pass:
Could you take a look at these when you get a chance? Thanks. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp (1)
7260-7266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
query_widget_propertiesbackend validation with the TypeScript payload contract.
query_widget_propertiescurrently hard-requiresproperties, but the TS handler path only guaranteeswidgetPathandwidgetName. This creates a cross-layer contract break and returnsMISSING_PARAMETERfor otherwise valid requests.Suggested fix
- const TArray<TSharedPtr<FJsonValue>>* PropsArray = GetArrayField(Payload, TEXT("properties")); - - if (WidgetPath.IsEmpty() || WidgetName.IsEmpty() || !PropsArray) + const TArray<TSharedPtr<FJsonValue>>* PropsArray = GetArrayField(Payload, TEXT("properties")); + + if (WidgetPath.IsEmpty() || WidgetName.IsEmpty()) { - SendAutomationError(RequestingSocket, RequestId, TEXT("Missing widgetPath, widgetName, or properties array"), TEXT("MISSING_PARAMETER")); + SendAutomationError(RequestingSocket, RequestId, TEXT("Missing widgetPath or widgetName"), TEXT("MISSING_PARAMETER")); return true; } @@ TArray<FString> Properties; - for (const auto& Val : *PropsArray) + if (PropsArray) { - Properties.Add(Val->AsString()); + for (const auto& Val : *PropsArray) + { + Properties.Add(Val->AsString()); + } }Also applies to: 7275-7279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp` around lines 7260 - 7266, The handler currently treats GetArrayField(Payload, TEXT("properties")) as required and returns MISSING_PARAMETER if PropsArray is null; change validation in the query_widget_properties handler so it only requires WidgetPath and WidgetName (i.e., remove PropsArray from the required-parameter check) and handle a missing PropsArray by treating it as an empty property list (e.g., create an empty TArray<TSharedPtr<FJsonValue>> or branch accordingly) before processing; update the same pattern at the other occurrence that uses PropsArray (the nearby block around the other SendAutomationError call) so both code paths accept requests that omit the properties field but still validate widgetPath/widgetName and proceed.
♻️ Duplicate comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp (1)
7286-7287:⚠️ Potential issue | 🟠 MajorGuard subsystem pointers before use in advanced UMG actions.
These branches check
GEditorbut still dereferenceGetEditorSubsystem<...>()without null-checking the returned subsystem pointer, which can crash instead of returning a structured automation error.As per coding guidelines, "Optional plugin features should fail gracefully when the UE module/plugin is unavailable".
#!/bin/bash # Verify all advanced UMG subsystem lookups and nearby guards. rg -n -C2 'GetEditorSubsystem<UUmg(Get|Set)Subsystem>' \ plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cppAlso applies to: 7319-7320, 7358-7359, 7391-7392, 7429-7430, 7458-7459
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp` around lines 7286 - 7287, The code dereferences the result of GEditor->GetEditorSubsystem<UUmgGetSubsystem>() (and similarly UUmgSetSubsystem) without checking for nullptr, which can crash; update each usage (e.g., the UUmgGetSubsystem* GetSubsystem = GEditor->GetEditorSubsystem<UUmgGetSubsystem>(); followed by GetSubsystem->QueryWidgetProperties(...)) to guard the subsystem pointer: fetch the subsystem into a local variable, if nullptr return/emit a structured automation error (same pattern used when GEditor is null), and only call QueryWidgetProperties or SetWidgetProperties when the subsystem pointer is valid; apply the same null-check fix to the other occurrences that call GetEditorSubsystem and then call QueryWidgetProperties, SetWidgetProperties, etc.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`:
- Around line 7260-7266: The handler currently treats GetArrayField(Payload,
TEXT("properties")) as required and returns MISSING_PARAMETER if PropsArray is
null; change validation in the query_widget_properties handler so it only
requires WidgetPath and WidgetName (i.e., remove PropsArray from the
required-parameter check) and handle a missing PropsArray by treating it as an
empty property list (e.g., create an empty TArray<TSharedPtr<FJsonValue>> or
branch accordingly) before processing; update the same pattern at the other
occurrence that uses PropsArray (the nearby block around the other
SendAutomationError call) so both code paths accept requests that omit the
properties field but still validate widgetPath/widgetName and proceed.
---
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cpp`:
- Around line 7286-7287: The code dereferences the result of
GEditor->GetEditorSubsystem<UUmgGetSubsystem>() (and similarly UUmgSetSubsystem)
without checking for nullptr, which can crash; update each usage (e.g., the
UUmgGetSubsystem* GetSubsystem =
GEditor->GetEditorSubsystem<UUmgGetSubsystem>(); followed by
GetSubsystem->QueryWidgetProperties(...)) to guard the subsystem pointer: fetch
the subsystem into a local variable, if nullptr return/emit a structured
automation error (same pattern used when GEditor is null), and only call
QueryWidgetProperties or SetWidgetProperties when the subsystem pointer is
valid; apply the same null-check fix to the other occurrences that call
GetEditorSubsystem and then call QueryWidgetProperties, SetWidgetProperties,
etc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f61c4645-8fc0-4c2b-9827-56c058e0f522
📒 Files selected for processing (5)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridge_WidgetAuthoringHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cppsrc/tools/consolidated-tool-handlers.ts
✅ Files skipped from review due to trivial changes (1)
- src/tools/consolidated-tool-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cpp
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cpp
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp
…ic handlers support via McpHandlers.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cpp`:
- Around line 1871-1890: RegisterHandler is overwriting built-in actions (e.g.,
console_command) so the dynamic JSON handler bypasses ConsoleCommandSecurity
checks; modify the lambda registration in
McpAutomationBridgeSubsystem::InitializeHandlers (or wherever
RegisterHandler(ActionName, ... ) is invoked) to first detect existing
authoritative handlers from InitializeHandlers (or the existing action map) and
refuse duplicate registration or reroute the ConsoleCommand case to the hardened
handler: for HandlerType == "ConsoleCommand" call the existing
HandleConsoleCommandAction (or delegate to the same validation path that uses
ConsoleCommandSecurity::IsBlockedCommand) instead of directly calling
ExecuteEditorCommands, otherwise return an error via SendAutomationError
indicating the action name is reserved.
- Around line 1914-1921: The code unconditionally dereferences P
(P.ToSharedRef()) when building PayloadStr which will assert if no payload is
provided; guard the payload by checking P.IsValid() (or P.IsValid() &&
P.IsValid() depending on type) before creating PayloadStr/Writer and calling
FJsonSerializer::Serialize, and only append the serialized-and-escaped argument
to PyCmd when the payload exists; update the block that creates PayloadStr,
TJsonWriter/Writer, and the PyCmd += FString::Printf(...) call to be inside an
if (P.IsValid()) { ... } branch so zero-argument handlers don't crash.
In `@src/tools/dynamic-handler-loader.ts`:
- Around line 45-63: The dynamic tool registration creates defs with inputSchema
but no outputSchema, breaking schema-backed outputs; update the registration
flow (where def is built and pushed to consolidatedToolDefinitions and
dynamicToolManager.registerDynamicTool is called) to attach a validated
outputSchema: look up the canonical output schema for actionName from the
central definitions collection (the same source-of-truth used for static tools),
set def.outputSchema = that schema (or fail/startup-error if missing), then
continue to push/register and keep toolRegistry.register(...) calling
executeAutomationRequest unchanged so runtime responses remain validated against
the registered outputSchema.
- Around line 15-26: The loader currently falls back to process.cwd() when
config.UE_PROJECT_PATH is unset causing mismatch with the bridge; change the
logic in dynamic-handler-loader so it requires config.UE_PROJECT_PATH and bails
out (throw an error or return early) instead of using the fallback.
Specifically, only compute configDir/jsonPath after verifying
config.UE_PROJECT_PATH is present, keep the existing .uproject dirname handling
for p when config.UE_PROJECT_PATH is set, and when it's missing throw or return
with a clear error/log rather than using process.cwd()/Config so the server and
plugin always read the same McpHandlers.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb43d5ba-43df-4e23-95d9-203c71aebdea
📒 Files selected for processing (11)
docs/Roadmap.mdplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/McpAutomationBridgeSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/McpAutomationBridgeSubsystem.hplugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgFileTransformation.hsrc/server-setup.tssrc/tools/dynamic-handler-loader.tssrc/tools/dynamic-tool-manager.ts
✅ Files skipped from review due to trivial changes (1)
- docs/Roadmap.md
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Public/UmgFileTransformation.h
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgFileTransformation.cpp
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgSetSubsystem.cpp
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgAttentionSubsystem.cpp
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/UmgGetSubsystem.cpp
- Prevent RegisterHandler from overwriting built-in authoritative actions - Guard P.ToSharedRef() when payload is empty in dynamic handlers - Ensure dynamic-handler-loader fails fast when UE_PROJECT_PATH is unset - Sync widget authoring actions to native schema registry
LuuOW
reviewed
Jun 17, 2026
LuuOW
left a comment
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: code patterns and architectural layout verified for system reliability.
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
3 participants
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
Integrates advanced Unreal Motion Graphics (UMG) authoring capabilities, allowing AI agents to create, query, and modify UI dynamically using complete JSON widget trees.
Changes
UUmgGetSubsystemandUUmgSetSubsystemfor property queries, layout extraction, and safe property modification.UUmgFileTransformationto serialize/deserialize complete UMG widget trees to/from JSON.McpAutomationBridge_WidgetAuthoringHandlers.cppwith new sub-actions (export_widget_tree,apply_widget_tree,query_widget_properties, etc.).consolidated-tool-definitions.tsand routing inwidget-authoring-handlers.ts.Related Issues
N/A
Type of Change
Testing
Pre-Merge Checklist