From 82f6fb949e1a79d3b6f9cb2231b1ce14d3e04072 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 8 Sep 2026 09:58:44 -0400 Subject: [PATCH 1/2] fix(central-ui): keep the instance picker disabled until the site's instances have loaded (typing raced the load and was wiped) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Pages/Deployment/DebugView.razor | 59 +++++++++++++------ .../Pages/Deployment/FlowDebugger.razor | 2 +- .../Pages/Deployment/FlowDebugger.razor.cs | 42 ++++++++++--- .../DebugViewInstancePickerTests.cs | 38 ++++++++++-- .../Deployment/FlowDebuggerPageTests.cs | 30 ++++++++++ 5 files changed, 138 insertions(+), 33 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/DebugView.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/DebugView.razor index 5b0b00ae..b5a041ca 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/DebugView.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/DebugView.razor @@ -85,7 +85,7 @@ + Disabled="@(_connected || _instancesLoading)" DataTestPrefix="debug-instance" />
@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 @@ } } + /// + /// Loads the selected site's instances and areas into the picker's two lists, holding + /// _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 + /// finally: a scope refusal or a repository fault must not leave the picker + /// disabled forever. + /// 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; } } diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor index d5ab097f..1dfb1a66 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor @@ -69,7 +69,7 @@ loaded once per site — the picker treats a new list as a site change. *@
diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor.cs index a2296c36..9e98afb1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor.cs @@ -122,6 +122,16 @@ public partial class FlowDebugger : ComponentBase, IDisposable /// The selected site's areas; the picker nests them into its tree. See . private List _areas = []; + + /// + /// True while 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 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). + /// + private bool _instancesLoading; private List _scripts = []; private int _selectedMethodId; @@ -419,19 +429,33 @@ public partial class FlowDebugger : ComponentBase, IDisposable await PredictAsync(); } + /// + /// Loads the selected site's instances and areas into the picker's two lists, holding + /// 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 + /// finally: a repository fault must not leave the picker disabled forever. + /// 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; + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs index 99486101..31b7e37c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs @@ -29,23 +29,24 @@ public class DebugViewInstancePickerTests : BunitContext { private const int SiteId = 1; + private readonly ITemplateEngineRepository _repo = Substitute.For(); + private void RegisterServices() { // DebugView touches localStorage on render; let bUnit answer loosely. JSInterop.Mode = JSRuntimeMode.Loose; - var repo = Substitute.For(); - repo.GetInstancesBySiteIdAsync(SiteId).Returns(new List + _repo.GetInstancesBySiteIdAsync(SiteId).Returns(new List { 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 + _repo.GetAreasBySiteIdAsync(SiteId).Returns(new List { 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(); siteRepo.GetAllSitesAsync().Returns(new List @@ -156,6 +157,35 @@ public class DebugViewInstancePickerTests : BunitContext Assert.False(connect.HasAttribute("disabled")); } + /// + /// The picker must stay disabled for the WHOLE site load, not just until the site id lands. + /// Blazor renders as soon as LoadInstancesForSite hits its first await, and at that point + /// the page is not _connected — so the field was already enabled while the site's lists + /// were still loading. Typing into it then (an operator, or Playwright's FillAsync, 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. + /// + [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>(); + _repo.GetInstancesBySiteIdAsync(SiteId).Returns(gate.Task); + + var cut = Render(); + 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() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs index ed738665..1b0e19d0 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs @@ -816,6 +816,36 @@ public class FlowDebuggerPageTests : BunitContext Assert.Equal("Sim01", cut.Find("[data-test='flow-instance-input']").GetAttribute("value"))); } + /// + /// The picker must stay disabled for the WHOLE site load, not just until the site id lands. + /// Blazor renders as soon as OnSiteChangedAsync hits its first await, and at that point + /// _selectedSiteId is already non-zero while _instances/_areas are still the + /// previous site's lists — so an operator (or Playwright's FillAsync, 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". + /// + [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>(); + _templates.GetInstancesBySiteIdAsync(1, Arg.Any()).Returns(gate.Task); + + var cut = Render(); + 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() { From 52726cf30d091a349c77904a84f389fd94c0e6ee Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 8 Sep 2026 10:01:22 -0400 Subject: [PATCH 2/2] fix(central-ui): Debug View reconnect guards the stored instance; drop select-era sort; deep-link-in-area test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three final-review findings on the two picker pages: 1. DebugView's localStorage auto-reconnect restored the stored instance id with no membership check, where the ?instanceId= path has one — a stored instance that is no longer Enabled (or was deleted, or moved sites) is filtered out of the list the picker is handed, so the field rendered blank while the page held a non-zero id and auto-connected to it anyway. The guard now mirrors the deep-link path: forget the stored instance id, tell the operator, and leave the site selected and loaded so they can pick again. 2. FlowDebugger's _instances kept an OrderBy(UniqueName) from the flat . + _instances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId)).ToList(); _areas = (await TemplateEngineRepository.GetAreasBySiteIdAsync(_selectedSiteId)).ToList(); } finally diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs index 31b7e37c..b7273e81 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/DebugViewInstancePickerTests.cs @@ -186,6 +186,31 @@ public class DebugViewInstancePickerTests : BunitContext Assert.False(cut.Find("[data-test='debug-instance-input']").HasAttribute("disabled"))); } + /// + /// The localStorage auto-reconnect must apply the same membership guard the + /// ?instanceId= 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. + /// + [Fact] + public void AutoReconnect_FromStorage_SkipsAnInstanceThatIsNoLongerAvailable() + { + RegisterServices(); + JSInterop.Setup("localStorage.getItem", "debugView.siteId").SetResult(SiteId.ToString()); + JSInterop.Setup("localStorage.getItem", "debugView.instanceId").SetResult("8"); + + var cut = Render(); + + 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() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs index 1b0e19d0..6bde8719 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs @@ -816,6 +816,37 @@ public class FlowDebuggerPageTests : BunitContext Assert.Equal("Sim01", cut.Find("[data-test='flow-instance-input']").GetAttribute("value"))); } + /// + /// The same deep link, but the instance sits two areas deep. The picker resolves the field + /// text from SelectedInstanceId 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). + /// + [Fact] + public void ScriptEntry_DeepLink_InstanceIdInsideAnArea_ShowsItsNameWithoutOpeningThePanel() + { + RegisterServices(); + WireInstance(); + _templates.GetAreasBySiteIdAsync(1, Arg.Any()) + .Returns(Task.FromResult>( + [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()) + .Returns(Task.FromResult>([nested])); + _templates.GetInstanceByIdAsync(7, Arg.Any()).Returns(nested); + + var nav = Services.GetRequiredService(); + nav.NavigateTo("/deployment/flow-debugger?instanceId=7"); + + var cut = Render(); + + 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']")); + } + /// /// The picker must stay disabled for the WHOLE site load, not just until the site id lands. /// Blazor renders as soon as OnSiteChangedAsync hits its first await, and at that point