Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ protected virtual void SetupSkillsUI()

// Show the skills output path
pathLabel.text = SkillsPath;
pathLabel.tooltip = SkillsPath;

// Configure toggle (per-agent)
toggleAutoGenerate.SetValueWithoutNotify(UnityMcpPluginEditor.IsAutoGenerateSkills(AgentId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ namespace com.IvanMurzak.Unity.MCP.Editor.UI
public class ConfigurationElements : IDisposable
{
public VisualElement Root { get; }
public VisualElement StatusCircle { get; }
public Label StatusText { get; }
public Button BtnConfigure { get; }

Expand All @@ -37,17 +36,33 @@ public ConfigurationElements(AiAgentConfig config, TransportMethod transportMode
_transportMode = transportMode;

Root = new UITemplate<VisualElement>("Editor/UI/uxml/agents/elements/TemplateConfigureStatus.uxml").Value;
StatusCircle = Root.Q<VisualElement>("configureStatusCircle") ?? throw new NullReferenceException("VisualElement 'configureStatusCircle' not found in UI.");
StatusText = Root.Q<Label>("configureStatusText") ?? throw new NullReferenceException("Label 'configureStatusText' not found in UI.");
BtnConfigure = Root.Q<Button>("btnConfigure") ?? throw new NullReferenceException("Button 'btnConfigure' not found in UI.");

var pathLabel = Root.Q<Label>("labelConfigPath");
if (pathLabel != null)
{
pathLabel.text = _config.ConfigPath;
pathLabel.tooltip = _config.ConfigPath;
}

UpdateStatus();

_clickCallback = new EventCallback<ClickEvent>(evt =>
{
var result = _config.Configure();
UpdateStatus(result);
onConfigured.OnNext(result);
var isCurrentlyConfigured = _config.IsConfigured();
if (isCurrentlyConfigured)
{
_config.Unconfigure();
UpdateStatus(false);
onConfigured.OnNext(false);
Comment on lines +57 to +58
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The remove branch assumes unconfiguration always succeeds and force-updates the UI/event state to "not configured". Since Unconfigure() can return false (e.g., file missing or write failure), this can desynchronize UI state from actual config state and emit incorrect status to subscribers. Re-read the actual configured state after attempting removal and publish that. [logic error]

Severity Level: Major ⚠️
- ⚠️ Agent configurator status can show false removal success.
- ⚠️ Remove/Configure button state can mismatch real file state.
- ⚠️ Debugging config failures becomes harder in editor UI.
Suggested change
UpdateStatus(false);
onConfigured.OnNext(false);
var isConfiguredAfter = _config.IsConfigured();
UpdateStatus(isConfiguredAfter);
onConfigured.OnNext(isConfiguredAfter);
Steps of Reproduction ✅
1. Open the Unity editor window through `MainWindowEditor.ShowWindow()`
(`Unity-MCP-Plugin/Assets/root/Editor/Scripts/UI/Window/MainWindowEditor.cs:31-38`), which
builds agent UI in `OnGUICreated()` and calls `ConfigureAgents(root)`
(`.../MainWindowEditor.CreateGUI.cs:184-197`).

2. `ConfigureAgents()` loads one agent panel with `LoadAgentUI()`
(`.../MainWindowEditor.AiAgents.cs:74,130-147`), which calls
`configurator.CreateUI(container)` (`.../MainWindowEditor.AiAgents.cs:143`) and constructs
`ConfigurationElements` with the remove/configure button
(`.../AiAgentConfigurators/ConfigurationElements.cs:33-40`).

3. Make the agent config file configured but non-writable (read-only/locked). This is a
real failure path: both config implementations perform `File.WriteAllText(...)` inside
`Unconfigure()` (`.../JsonAiAgentConfig.cs:257`, `.../TomlAiAgentConfig.cs:235`) and
return `false` on exception (`.../JsonAiAgentConfig.cs:260-264`,
`.../TomlAiAgentConfig.cs:238-242`).

4. Click **Remove**. In `ConfigurationElements` callback, code executes
`_config.Unconfigure(); UpdateStatus(false); onConfigured.OnNext(false);`
(`.../ConfigurationElements.cs:53-58`) without checking outcome. Result: UI flips to "Not
Configured" even though unconfigure failed and file stayed configured.

5. This mismatch is technically observable in current flow because `Unconfigure()` is
explicitly designed to fail in realistic conditions and return false (also validated by
tests expecting false: `.../Tests/Editor/UI/JsonAiAgentConfigTests.cs:1312-1331`,
`.../Tests/Editor/UI/TomlAiAgentConfigTests.cs:1141-1152`).
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** Unity-MCP-Plugin/Assets/root/Editor/Scripts/UI/AiAgentConfigurators/ConfigurationElements.cs
**Line:** 57:58
**Comment:**
	*Logic Error: The remove branch assumes unconfiguration always succeeds and force-updates the UI/event state to "not configured". Since `Unconfigure()` can return `false` (e.g., file missing or write failure), this can desynchronize UI state from actual config state and emit incorrect status to subscribers. Re-read the actual configured state after attempting removal and publish that.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
👍 | 👎

}
else
{
var result = _config.Configure();
UpdateStatus(result);
onConfigured.OnNext(result);
}
Comment on lines +53 to +65
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Check Unconfigure() return value to avoid UI state mismatch.

The Unconfigure() method returns bool to indicate success/failure (see TomlAiAgentConfig.Unconfigure() in the codebase), but the return value is ignored here. If unconfiguration fails, the UI will incorrectly show "Not Configured" even though the configuration may still exist.

🛠️ Suggested fix
 if (isCurrentlyConfigured)
 {
-    _config.Unconfigure();
-    UpdateStatus(false);
-    onConfigured.OnNext(false);
+    var unconfigured = _config.Unconfigure();
+    if (unconfigured)
+    {
+        UpdateStatus(false);
+        onConfigured.OnNext(false);
+    }
+    // Optionally: else show error feedback
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@Unity-MCP-Plugin/Assets/root/Editor/Scripts/UI/AiAgentConfigurators/ConfigurationElements.cs`
around lines 53 - 65, When unconfiguring, check the boolean return from
_config.Unconfigure() instead of ignoring it: call var success =
_config.Unconfigure(); then call UpdateStatus(!success ? _config.IsConfigured()
: false) or simply UpdateStatus(!success) based on desired semantics and emit
onConfigured.OnNext(success) so the UI reflects the actual result; update the
block that currently calls _config.Unconfigure() to use the returned bool and
propagate it to UpdateStatus(...) and onConfigured.OnNext(...).

});
BtnConfigure.RegisterCallback(_clickCallback);
}
Expand All @@ -64,16 +79,9 @@ public void UpdateStatus(bool? isConfigured = null)

StatusText.text = isConfiguredValue ? $"Configured ({transportText})" : "Not Configured";

StatusCircle.RemoveFromClassList(MainWindowEditor.USS_Connected);
StatusCircle.RemoveFromClassList(MainWindowEditor.USS_Connecting);
StatusCircle.RemoveFromClassList(MainWindowEditor.USS_Disconnected);
StatusCircle.AddToClassList(isConfiguredValue
? MainWindowEditor.USS_Connected
: MainWindowEditor.USS_Disconnected);

BtnConfigure.text = isConfiguredValue ? "Reconfigure" : "Configure";
BtnConfigure.text = isConfiguredValue ? "Remove" : "Configure";
BtnConfigure.EnableInClassList("btn-primary", !isConfiguredValue);
BtnConfigure.EnableInClassList("btn-secondary", isConfiguredValue);
BtnConfigure.EnableInClassList("btn-alert", isConfiguredValue);
}

public void Dispose()
Expand Down
8 changes: 8 additions & 0 deletions Unity-MCP-Plugin/Assets/root/Editor/Scripts/UI/Controls.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/*
┌──────────────────────────────────────────────────────────────────┐
│ Author: Ivan Murzak (https://github.com/IvanMurzak) │
│ Repository: GitHub (https://github.com/IvanMurzak/Unity-MCP) │
│ Copyright (c) 2025 Ivan Murzak │
│ Licensed under the Apache License, Version 2.0. │
│ See the LICENSE file in the project root for more information. │
└──────────────────────────────────────────────────────────────────┘
*/

#nullable enable
using System;
using System.Collections.Generic;
using com.IvanMurzak.Unity.MCP.Editor.Utils;
using UnityEngine.UIElements;

namespace com.IvanMurzak.Unity.MCP.Editor.UI.Controls
{
/// <summary>
/// A segmented control with N segments and a sliding highlight.
/// Implements <see cref="INotifyValueChanged{T}"/> for UI Toolkit integration.
/// All visual styling is defined in <c>_segmented-control.uss</c>.
/// </summary>
public class SegmentedControl : VisualElement, INotifyValueChanged<int>
{
private static readonly string[] UssPaths =
EditorAssetLoader.GetEditorAssetPaths("Editor/UI/uss/common/_segmented-control.uss");

public new class UxmlFactory : UxmlFactory<SegmentedControl, UxmlTraits> { }

public new class UxmlTraits : VisualElement.UxmlTraits
{
private readonly UxmlStringAttributeDescription _choices = new() { name = "choices", defaultValue = "" };
private readonly UxmlIntAttributeDescription _defaultIndex = new() { name = "default-index", defaultValue = 0 };

public override void Init(VisualElement ve, IUxmlAttributes bag, CreationContext cc)
{
base.Init(ve, bag, cc);
var control = (SegmentedControl)ve;
var choicesValue = _choices.GetValueFromBag(bag, cc);
var defaultIndex = _defaultIndex.GetValueFromBag(bag, cc);

if (!string.IsNullOrEmpty(choicesValue))
{
var labels = choicesValue.Split(',');
for (var i = 0; i < labels.Length; i++)
labels[i] = labels[i].Trim();
control.BuildSegments(labels);
control.SetValueWithoutNotify(defaultIndex);
}
}
}

private readonly VisualElement _track;
private readonly VisualElement _highlight;
private readonly List<Label> _segments = new();
private int _selectedIndex;
private bool _transitionReady;

public int SelectedIndex
{
get => _selectedIndex;
set => SetValue(value);
}

int INotifyValueChanged<int>.value
{
get => _selectedIndex;
set => SetValue(value);
}

public SegmentedControl() : this(Array.Empty<string>()) { }

public SegmentedControl(params string[] labels)
{
AddToClassList("segmented-control");

var uss = EditorAssetLoader.LoadAssetAtPath<StyleSheet>(UssPaths);
if (uss != null)
styleSheets.Add(uss);

_track = new VisualElement();
_track.AddToClassList("segmented-control__track");
Add(_track);

_highlight = new VisualElement();
_highlight.AddToClassList("segmented-control__highlight");
_highlight.AddToClassList("segmented-control__highlight--no-transition");
_track.Add(_highlight);

if (labels.Length > 0)
BuildSegments(labels);
}

private void BuildSegments(string[] labels)
{
// Remove old segments (keep highlight at index 0)
foreach (var seg in _segments)
_track.Remove(seg);
_segments.Clear();

for (var i = 0; i < labels.Length; i++)
{
var label = new Label(labels[i]);
label.AddToClassList("segmented-control__segment");
var index = i;
label.RegisterCallback<ClickEvent>(_ => SelectedIndex = index);
label.RegisterCallback<GeometryChangedEvent>(_ => SnapHighlightToSegment());
_track.Add(label);
_segments.Add(label);
}

_transitionReady = false;
UpdateHighlight();
}

/// <summary>
/// Sets per-segment tooltips. Segment count must match.
/// </summary>
public void SetTooltips(params string[] tooltips)
{
for (var i = 0; i < tooltips.Length && i < _segments.Count; i++)
_segments[i].tooltip = tooltips[i];
}

public void SetValueWithoutNotify(int newValue)
{
if (newValue < 0 || (_segments.Count > 0 && newValue >= _segments.Count))
return;

_selectedIndex = newValue;
UpdateHighlight();
}

private void SetValue(int newValue)
{
if (newValue < 0 || (_segments.Count > 0 && newValue >= _segments.Count))
return;
if (newValue == _selectedIndex)
return;

using var evt = ChangeEvent<int>.GetPooled(_selectedIndex, newValue);
evt.target = this;
_selectedIndex = newValue;
UpdateHighlight();
SendEvent(evt);
}

/// <summary>
/// Positions the highlight to match the selected segment's actual layout.
/// Called on geometry changes and selection changes.
/// </summary>
private void SnapHighlightToSegment()
{
if (_segments.Count == 0 || _selectedIndex < 0 || _selectedIndex >= _segments.Count)
return;

var segment = _segments[_selectedIndex];
var x = segment.layout.x;
var w = segment.layout.width;

// layout is zero before first layout pass
if (float.IsNaN(w) || w <= 0)
return;

_highlight.style.left = x;
_highlight.style.width = w;
}

private void UpdateHighlight()
{
if (_segments.Count == 0)
return;

if (!_transitionReady)
{
// Start with transitions suppressed; enable after first layout
SnapHighlightToSegment();
schedule.Execute(() =>
{
_transitionReady = true;
_highlight.RemoveFromClassList("segmented-control__highlight--no-transition");
});
}
else
{
SnapHighlightToSegment();
}

for (var i = 0; i < _segments.Count; i++)
_segments[i].EnableInClassList("segmented-control__segment--selected", i == _selectedIndex);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading