Merge branch 'worktree-agent-a327a30cdb2e454ff' into feat/instance-picker

2026-09-08 10:01:47 -04:00
parent 4df0cec8d8 52726cf30d
commit 50eca87e99
5 changed files with 212 additions and 35 deletions
@@ -85,7 +85,7 @@
                <InstancePicker Areas="_siteAreas" Instances="_siteInstances"
                                ItemLabel="InstanceLabel"
                                SelectedInstanceId="_selectedInstanceId" SelectedInstanceIdChanged="OnInstancePicked"
-                                Disabled="@_connected" DataTestPrefix="debug-instance" />
+                                Disabled="@(_connected || _instancesLoading)" DataTestPrefix="debug-instance" />
            </div>
            <div class="col-md-3 d-flex align-items-end gap-2">
                @if (!_connected)
@@ -263,6 +263,12 @@
    private bool _loading = true;
    private bool _connected;
    private bool _connecting;
+
+    // True for the whole of LoadInstancesForSite. Blazor renders as soon as that load hits its
+    // first await, and the page is not _connected then — so without this the picker is enabled
+    // while it is still showing the PREVIOUS site's tree, and anything typed into it is wiped
+    // when the new lists land (the picker reads a new list object as "the site changed").
+    private bool _instancesLoading;
    private bool _connectedFromStorage;

    // Which tab pane is visible — "attributes" (default) or "alarms".
@@ -415,43 +421,74 @@
        {
            _selectedSiteId = siteId;
            await LoadInstancesForSite();
+
+            // Membership guard, mirroring the ?instanceId= path above. A stored instance that
+            // has since been disabled (or deleted, or moved to another site) is not in the list
+            // the picker was handed, so it renders a blank field while the page holds a non-zero
+            // id and auto-connects to it anyway. Forget the stored instance instead — the site
+            // stays selected and loaded, so the operator just picks again.
+            var stored = _siteInstances.FirstOrDefault(i => i.Id == instanceId);
+            if (stored == null)
+            {
+                await JS.InvokeVoidAsync("localStorage.removeItem", "debugView.instanceId");
+                _toast.ShowInfo(
+                    "The instance from your previous session is no longer available for debug streaming.",
+                    autoDismissMs: 8000);
+                StateHasChanged();
+                return;
+            }
+
            _selectedInstanceId = instanceId;
            _connectedFromStorage = true;
            StateHasChanged();
            await Connect();

            // Auto-reconnect notice — the user didn't initiate this connection.
-            var inst = _siteInstances.FirstOrDefault(i => i.Id == instanceId);
            _toast.ShowInfo(
-                $"Auto-reconnected to {inst?.UniqueName ?? "instance"} from previous session.",
+                $"Auto-reconnected to {stored.UniqueName} from previous session.",
                autoDismissMs: 8000);
        }
    }

+    /// <summary>
+    /// Loads the selected site's instances and areas into the picker's two lists, holding
+    /// <c>_instancesLoading</c> for the whole round trip so the field cannot be typed into
+    /// while it is still showing the previous site's tree. The flag is cleared in a
+    /// <c>finally</c>: a scope refusal or a repository fault must not leave the picker
+    /// disabled forever.
+    /// </summary>
    private async Task LoadInstancesForSite()
    {
-        _siteInstances = new();
-        _siteAreas = new();
-        _selectedInstanceId = 0;
-        if (_selectedSiteId == 0) return;
-        // Site scoping (CentralUI-002): re-check the claim server-side — a query
-        // string or stale localStorage value could name a site outside the grant.
-        if (!await SiteScope.IsSiteAllowedAsync(_selectedSiteId))
-        {
-            _selectedSiteId = 0;
-            _toast.ShowError("You are not permitted to debug instances on that site.");
-            return;
-        }
+        _instancesLoading = true;
        try
        {
-            _siteInstances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId))
-                .Where(i => i.State == InstanceState.Enabled)
-                .ToList();
-            _siteAreas = (await TemplateEngineRepository.GetAreasBySiteIdAsync(_selectedSiteId)).ToList();
+            _siteInstances = new();
+            _siteAreas = new();
+            _selectedInstanceId = 0;
+            if (_selectedSiteId == 0) return;
+            // Site scoping (CentralUI-002): re-check the claim server-side — a query
+            // string or stale localStorage value could name a site outside the grant.
+            if (!await SiteScope.IsSiteAllowedAsync(_selectedSiteId))
+            {
+                _selectedSiteId = 0;
+                _toast.ShowError("You are not permitted to debug instances on that site.");
+                return;
+            }
+            try
+            {
+                _siteInstances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId))
+                    .Where(i => i.State == InstanceState.Enabled)
+                    .ToList();
+                _siteAreas = (await TemplateEngineRepository.GetAreasBySiteIdAsync(_selectedSiteId)).ToList();
+            }
+            catch (Exception ex)
+            {
+                _toast.ShowError($"Failed to load instances: {ex.Message}");
+            }
        }
-        catch (Exception ex)
+        finally
        {
-            _toast.ShowError($"Failed to load instances: {ex.Message}");
+            _instancesLoading = false;
        }
    }

@@ -69,7 +69,7 @@
                       loaded once per site — the picker treats a new list as a site change. *@
                    <InstancePicker Areas="_areas" Instances="_instances"
                                    SelectedInstanceId="_selectedInstanceId" SelectedInstanceIdChanged="OnInstancePickedAsync"
-                                    Disabled="@(_running || _selectedSiteId == 0)"
+                                    Disabled="@(_running || _selectedSiteId == 0 || _instancesLoading)"
                                    Placeholder="Search instances…" DataTestPrefix="flow-instance" />
                </div>
                <div class="col-md-3">
@@ -122,6 +122,16 @@ public partial class FlowDebugger : ComponentBase, IDisposable

    /// <summary>The selected site's areas; the picker nests them into its tree. See <see cref="_instances"/>.</summary>
    private List<Area> _areas = [];
+
+    /// <summary>
+    /// True while <see cref="LoadInstancesAsync"/> is fetching a site's instances and areas, and
+    /// the sole reason the picker is disabled for that window. Blazor renders the moment the load
+    /// hits its first await, and by then <see cref="_selectedSiteId"/> is already the NEW site
+    /// while the two lists are still the OLD ones — so without this the field is enabled over
+    /// stale data, and anything typed into it is wiped when the lists land (the picker reads a new
+    /// list object as "the site changed" and clears its text).
+    /// </summary>
+    private bool _instancesLoading;
    private List<FlowScriptOption> _scripts = [];

    private int _selectedMethodId;
@@ -419,19 +429,33 @@ public partial class FlowDebugger : ComponentBase, IDisposable
            await PredictAsync();
    }

+    /// <summary>
+    /// Loads the selected site's instances and areas into the picker's two lists, holding
+    /// <see cref="_instancesLoading"/> for the whole round trip so the field cannot be typed
+    /// into while it is still showing the previous site's tree. The flag is cleared in a
+    /// <c>finally</c>: a repository fault must not leave the picker disabled forever.
+    /// </summary>
    private async Task LoadInstancesAsync()
    {
-        if (_selectedSiteId <= 0)
+        _instancesLoading = true;
+        try
        {
-            _instances = [];
-            _areas = [];
-            return;
-        }
+            if (_selectedSiteId <= 0)
+            {
+                _instances = [];
+                _areas = [];
+                return;
+            }

-        _instances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId))
-            .OrderBy(i => i.UniqueName, StringComparer.OrdinalIgnoreCase).ToList();
-        // The picker nests these itself (InstanceTreeBuilder), so order does not matter here.
-        _areas = (await TemplateEngineRepository.GetAreasBySiteIdAsync(_selectedSiteId)).ToList();
+            // Neither list is sorted here: the picker nests and orders both itself
+            // (InstanceTreeBuilder). The OrderBy this replaces dated from the flat <select>.
+            _instances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId)).ToList();
+            _areas = (await TemplateEngineRepository.GetAreasBySiteIdAsync(_selectedSiteId)).ToList();
+        }
+        finally
+        {
+            _instancesLoading = false;
+        }
    }

    /// <summary>
@@ -29,23 +29,24 @@ public class DebugViewInstancePickerTests : BunitContext
 {
    private const int SiteId = 1;

+    private readonly ITemplateEngineRepository _repo = Substitute.For<ITemplateEngineRepository>();
+
    private void RegisterServices()
    {
        // DebugView touches localStorage on render; let bUnit answer loosely.
        JSInterop.Mode = JSRuntimeMode.Loose;

-        var repo = Substitute.For<ITemplateEngineRepository>();
-        repo.GetInstancesBySiteIdAsync(SiteId).Returns(new List<Instance>
+        _repo.GetInstancesBySiteIdAsync(SiteId).Returns(new List<Instance>
        {
            new("Sim01") { Id = 7, SiteId = SiteId, AreaId = 2, State = InstanceState.Enabled },
            new("Off01") { Id = 8, SiteId = SiteId, State = InstanceState.Disabled },
        });
-        repo.GetAreasBySiteIdAsync(SiteId).Returns(new List<Area>
+        _repo.GetAreasBySiteIdAsync(SiteId).Returns(new List<Area>
        {
            new("Line1") { Id = 1, SiteId = SiteId },
            new("Cell A") { Id = 2, SiteId = SiteId, ParentAreaId = 1 },
        });
-        Services.AddSingleton(repo);
+        Services.AddSingleton(_repo);

        var siteRepo = Substitute.For<ISiteRepository>();
        siteRepo.GetAllSitesAsync().Returns(new List<Site>
@@ -156,6 +157,60 @@ public class DebugViewInstancePickerTests : BunitContext
        Assert.False(connect.HasAttribute("disabled"));
    }

+    /// <summary>
+    /// The picker must stay disabled for the WHOLE site load, not just until the site id lands.
+    /// Blazor renders as soon as <c>LoadInstancesForSite</c> hits its first await, and at that point
+    /// the page is not <c>_connected</c> — so the field was already enabled while the site's lists
+    /// were still loading. Typing into it then (an operator, or Playwright's <c>FillAsync</c>, which
+    /// waits only for "enabled") lost the text: the picker reads the freshly-assigned lists as
+    /// "the site changed" and clears the field and closes the panel.
+    /// </summary>
+    [Fact]
+    public void PickerStaysDisabled_UntilTheSitesInstancesHaveLoaded()
+    {
+        RegisterServices();
+        // Hold the instance load open so the render that follows the first await is observable.
+        var gate = new TaskCompletionSource<IReadOnlyList<Instance>>();
+        _repo.GetInstancesBySiteIdAsync(SiteId).Returns(gate.Task);
+
+        var cut = Render<DebugViewPage>();
+        cut.Find("[data-test='debug-site-select']").Change(SiteId.ToString());
+
+        Assert.True(cut.Find("[data-test='debug-instance-input']").HasAttribute("disabled"));
+        Assert.True(cut.Find("[data-test='debug-instance-toggle']").HasAttribute("disabled"));
+
+        cut.InvokeAsync(() => gate.SetResult(
+            [new Instance("Sim01") { Id = 7, SiteId = SiteId, AreaId = 2, State = InstanceState.Enabled }]));
+
+        cut.WaitForAssertion(() =>
+            Assert.False(cut.Find("[data-test='debug-instance-input']").HasAttribute("disabled")));
+    }
+
+    /// <summary>
+    /// The localStorage auto-reconnect must apply the same membership guard the
+    /// <c>?instanceId=</c> deep link does. Off01 is Disabled, so the page filters it out of the
+    /// list the picker is handed — restoring it anyway left the field blank while the page held
+    /// a non-zero id and auto-connected to it. The stored id is dropped instead, and the site
+    /// stays selected and loaded so the operator can pick again.
+    /// </summary>
+    [Fact]
+    public void AutoReconnect_FromStorage_SkipsAnInstanceThatIsNoLongerAvailable()
+    {
+        RegisterServices();
+        JSInterop.Setup<string>("localStorage.getItem", "debugView.siteId").SetResult(SiteId.ToString());
+        JSInterop.Setup<string>("localStorage.getItem", "debugView.instanceId").SetResult("8");
+
+        var cut = Render<DebugViewPage>();
+
+        cut.WaitForAssertion(() =>
+            Assert.Equal(string.Empty, cut.Find("[data-test='debug-instance-input']").GetAttribute("value")));
+
+        // Nothing is selected, so Connect stays disabled — and the stale id is forgotten.
+        var connect = cut.FindAll("button.btn.btn-primary.btn-sm").First(b => b.TextContent.Contains("Connect"));
+        Assert.True(connect.HasAttribute("disabled"));
+        JSInterop.VerifyInvoke("localStorage.removeItem");
+    }
+
    [Fact]
    public void WhileConnected_ThePickerIsDisabled()
    {
@@ -816,6 +816,67 @@ public class FlowDebuggerPageTests : BunitContext
            Assert.Equal("Sim01", cut.Find("[data-test='flow-instance-input']").GetAttribute("value")));
    }

+    /// <summary>
+    /// The same deep link, but the instance sits two areas deep. The picker resolves the field
+    /// text from <c>SelectedInstanceId</c> against the flat instance list, not by walking the
+    /// tree — so a nested instance must show its name with the panel never opened (and therefore
+    /// with Line1 / Cell A never expanded).
+    /// </summary>
+    [Fact]
+    public void ScriptEntry_DeepLink_InstanceIdInsideAnArea_ShowsItsNameWithoutOpeningThePanel()
+    {
+        RegisterServices();
+        WireInstance();
+        _templates.GetAreasBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<IReadOnlyList<Area>>(
+                [new Area("Line1") { Id = 1, SiteId = 1 }, new Area("Cell A") { Id = 2, SiteId = 1, ParentAreaId = 1 }]));
+        var nested = new Instance("Sim01") { Id = 7, SiteId = 1, TemplateId = 10, AreaId = 2 };
+        _templates.GetInstancesBySiteIdAsync(1, Arg.Any<CancellationToken>())
+            .Returns(Task.FromResult<IReadOnlyList<Instance>>([nested]));
+        _templates.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>()).Returns(nested);
+
+        var nav = Services.GetRequiredService<NavigationManager>();
+        nav.NavigateTo("/deployment/flow-debugger?instanceId=7");
+
+        var cut = Render<FlowDebuggerPage>();
+
+        cut.WaitForAssertion(() =>
+            Assert.Equal("Sim01", cut.Find("[data-test='flow-instance-input']").GetAttribute("value")));
+        // The panel was never opened, so nothing of the tree is rendered.
+        Assert.Empty(cut.FindAll("[data-test='flow-instance-panel']"));
+        Assert.Empty(cut.FindAll("[data-test='flow-instance-row']"));
+    }
+
+    /// <summary>
+    /// The picker must stay disabled for the WHOLE site load, not just until the site id lands.
+    /// Blazor renders as soon as <c>OnSiteChangedAsync</c> hits its first await, and at that point
+    /// <c>_selectedSiteId</c> is already non-zero while <c>_instances</c>/<c>_areas</c> are still the
+    /// previous site's lists — so an operator (or Playwright's <c>FillAsync</c>, which waits only for
+    /// "enabled") could type into the field and have the load wipe the text and close the panel a
+    /// moment later, because the picker reads a new list object as "the site changed".
+    /// </summary>
+    [Fact]
+    public void ScriptEntry_PickerStaysDisabled_UntilTheSitesInstancesHaveLoaded()
+    {
+        RegisterServices();
+        WireInstance();
+        // Hold the instance load open so the render that follows the first await is observable.
+        var gate = new TaskCompletionSource<IReadOnlyList<Instance>>();
+        _templates.GetInstancesBySiteIdAsync(1, Arg.Any<CancellationToken>()).Returns(gate.Task);
+
+        var cut = Render<FlowDebuggerPage>();
+        cut.Find("button[data-test='flow-entry-script']").Click();
+        cut.Find("select[data-test='flow-site-select']").Change("1");
+
+        Assert.True(cut.Find("[data-test='flow-instance-input']").HasAttribute("disabled"));
+        Assert.True(cut.Find("[data-test='flow-instance-toggle']").HasAttribute("disabled"));
+
+        cut.InvokeAsync(() => gate.SetResult([new Instance("Sim01") { Id = 7, SiteId = 1, TemplateId = 10 }]));
+
+        cut.WaitForAssertion(() =>
+            Assert.False(cut.Find("[data-test='flow-instance-input']").HasAttribute("disabled")));
+    }
+
    [Fact]
    public void ScriptEntry_SiteChange_ClearsThePickedInstance()
    {