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();