fix(central-ui): keep the instance picker disabled until the site's instances have loaded (typing raced the load and was wiped)

Both Flow Debugger and Debug View enable the shared InstancePicker as soon as _selectedSiteId lands, but Blazor renders at the site-change handler's first await — at which point the picker is still holding the PREVIOUS site's Areas / Instances lists. Typing into the field in that window (an operator, or Playwright's FillAsync, which waits only for "enabled") was silently lost: the picker compares both lists by reference, so the freshly-assigned lists read as "the site changed" and it clears the field text and closes the panel. Both pages now hold an _instancesLoading flag across the whole load — set before the first await, cleared in a finally so a fault or a site-scope refusal cannot leave the picker disabled forever — and fold it into the picker's Disabled expression. InstancePicker itself is unchanged. Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
2026-09-08 09:58:44 -04:00
parent d3264ec1b1
commit 82f6fb949e
5 changed files with 138 additions and 33 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".
@@ -428,30 +434,45 @@
        }
    }

+    /// <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();
+            _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();
+        }
+        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,35 @@ 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")));
+    }
+
    [Fact]
    public void WhileConnected_ThePickerIsDisabled()
    {
@@ -816,6 +816,36 @@ public class FlowDebuggerPageTests : BunitContext
            Assert.Equal("Sim01", cut.Find("[data-test='flow-instance-input']").GetAttribute("value")));
    }

+    /// <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()
    {