From d14e0ee4b19b2073ef93863d356d88963ba3210e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 11 Aug 2026 06:03:06 -0400 Subject: [PATCH] fix(ui): gate detail modals on user intent, not on the row resolving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep's modal re-key (holding the row's id and re-resolving it, rather than holding the record) also used that resolve as the modal's visibility gate. That makes the modal's existence a function of list contents: any render where the row is momentarily unresolvable unmounts the whole subtree and disposes every event-handler id inside it, Close's included. A click already in flight against a disposed handler makes the renderer throw GetRequiredEventBindingEntry during DispatchEventAsync — which is how this surfaced, as an intermittent failure of CloseButton_DismissesModal (989/990 on one run, green on re-run). The record-held form made that structurally impossible: the modal existed because the user opened it, and no list mutation could retract that. This restores the property while keeping the re-key's actual benefit. Visibility now gates on the held id; the resolve drives only content. An unresolvable row degrades to an explicit notice and hides the row-scoped actions, while the frame and Close stay mounted. Detail fetched by id still renders, so the user does not lose the body they opened. Applied to all four surfaces that shared the construction: NotificationReport, ConfigurationAuditLog, ParkedMessages (offcanvas drawer) and SiteCallsReport. Modal_StaysOpen_WhenItsRowLeavesThePage drops the opened row from the next query and asserts the modal survives, keeps its fetched body, hides Retry/Discard, and that Close still works. It was run against a deliberately restored defective gate and failed there before passing here — a regression test that passes both ways would be worthless against a race. 20 consecutive runs of the previously flaky class: no failures. CentralUI.Tests 991/991, solution build 0/0. The plan doc gains a section recording that the sweep was reported as behaviour-preserving when it was not, and why the merge review missed it. --- .../2026-08-11-central-ui-cleanup-sweep.md | 67 +++++++++++++++++++ .../Pages/Audit/ConfigurationAuditLog.razor | 30 +++++++-- .../Pages/Monitoring/ParkedMessages.razor | 58 +++++++++++----- .../Notifications/NotificationReport.razor | 48 ++++++++++--- .../Pages/SiteCalls/SiteCallsReport.razor | 27 +++++--- .../NotificationReportDetailModalTests.cs | 53 +++++++++++++++ 6 files changed, 238 insertions(+), 45 deletions(-) diff --git a/docs/plans/2026-08-11-central-ui-cleanup-sweep.md b/docs/plans/2026-08-11-central-ui-cleanup-sweep.md index f7f1a4b7..672195ed 100644 --- a/docs/plans/2026-08-11-central-ui-cleanup-sweep.md +++ b/docs/plans/2026-08-11-central-ui-cleanup-sweep.md @@ -290,3 +290,70 @@ equivalent re-key on NotificationReport was exercised through the confirm path a **Rig note:** `docker/docker-compose.yml` carries an uncommitted `DisableLogin: "true"` from the earlier EWS live-gate session (SEC-36). It predates this sweep and was left untouched. + +## 5. Post-sweep correction: the modal re-key was not behaviour-preserving + +This sweep was carried out and reported as "wrappers, classes and prose only — +behaviour preserved byte-for-byte." **That claim was false for four files**, and the +error was found only after the fact, while re-reading the same files during the +Theme 0.4.1 bump. Recorded here rather than quietly fixed, because the failure was +in the reporting as much as in the code. + +### What actually changed + +Four detail surfaces were re-keyed from holding the selected **record** to holding +its **id** and re-resolving from the current page on every render: + +| Surface | Field before | After | +|---|---|---| +| `Pages/Notifications/NotificationReport.razor` | `_detailNotification` | `_detailNotificationId` + `DetailRow()` | +| `Pages/Audit/ConfigurationAuditLog.razor` | `_modalEntry` | `_modalEntryId` + `ModalEntry()` | +| `Pages/Monitoring/ParkedMessages.razor` | (record) | `_drawerMessageId` + `DrawerMessage` | +| `Pages/SiteCalls/SiteCallsReport.razor(.cs)` | `_detailSiteCall` | `_detailSiteCallId` + `DetailRow()` | + +The re-key itself is sound — it fixes a real stale-record class of bug. The defect +was in what it was wired to. + +### The defect + +Each surface also used the resolve as the **visibility gate** +(`@if (DetailRow() is { } d)`). That makes the modal's *existence* a function of +list contents, so any render where the row is momentarily unresolvable unmounts the +entire subtree and disposes every event-handler id inside it — including Close's. A +click already in flight against a disposed handler makes the renderer throw +`GetRequiredEventBindingEntry` during `DispatchEventAsync`. + +The previous field-held record made this structurally impossible: the modal existed +because the user opened it, and no list mutation could retract that. + +**Symptom:** an intermittent `NotificationReportDetailModalTests.CloseButton_DismissesModal` +failure — 989/990 on one full-suite run, passing on re-run and 5/5 in isolation. +That signature is a race, not evidence against one. + +### The fix + +Visibility is now gated on the held **id** (user intent); the resolve drives only +*content*. An unresolvable row degrades to an explicit notice and hides the +row-scoped actions, while the frame and Close stay mounted. Detail fetched by id +(`_detail`) is unaffected and still renders. + +### Verification + +- `Modal_StaysOpen_WhenItsRowLeavesThePage` — opens the modal, drops the row from + the next query, asserts the modal is still mounted, shows the notice, keeps the + fetched body, hides Retry/Discard, and that Close still works. +- **Negative control:** the test was run against a deliberately restored defective + gate and **failed**, then passed on the fix. A regression test that passes both + ways proves nothing; this one discriminates. +- 20 consecutive runs of the previously-flaky class: 0 failures. +- CentralUI.Tests 991/991; full solution build 0 warnings / 0 errors. + +### Process note + +The sweep ran as parallel batch agents, several of which correctly refused +instructions that were wrong (§2 Refusals). This defect went the other way: an agent +did *more* than the brief, the extra work looked like an improvement, and the +merge review checked that the diff was *plausible* rather than that it was *in +scope*. A scope claim covering N files needs to be checked against N diffs — for +this sweep, `git diff` filtered to non-comment, non-class changes takes about two +minutes and would have caught it before the commit, not after. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Audit/ConfigurationAuditLog.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Audit/ConfigurationAuditLog.razor index bb691735..f6f89c9d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Audit/ConfigurationAuditLog.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Audit/ConfigurationAuditLog.razor @@ -180,24 +180,40 @@ } @* The state modal holds only the entry's id and re-resolves the row from the - page currently on screen on every render (ModalEntry()). A refetch that - replaces the entry list therefore never leaves this surface rendering a - stale record, and an entry that has left the page closes the modal instead - of stranding it. *@ - @if (ModalEntry() is { } modalEntry) + page currently on screen on every render (ModalEntry()), so it never + renders a stale record after a refetch replaces the entry list. + + Visibility is gated on _modalEntryId — user intent — NOT on the resolve + succeeding. Gating the subtree on ModalEntry() would make the modal's + existence a function of list contents, so a render where the entry is + momentarily absent unmounts the subtree and disposes the event-handler ids + inside it, Close included; a click already in flight against a disposed + handler throws GetRequiredEventBindingEntry. Only the user closes this. *@ + @if (_modalEntryId is { } modalEntryId) { + var modalEntry = ModalEntry();