fix(ui): gate detail modals on user intent, not on the row resolving

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 06:03:06 -04:00
parent 9e243493fb
commit d14e0ee4b1
6 changed files with 238 additions and 45 deletions
@@ -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.
@@ -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();
        <div class="modal-backdrop fade show"></div>
        <div class="modal fade show d-block" tabindex="-1" role="dialog">
            <div class="modal-dialog modal-lg modal-dialog-centered modal-dialog-scrollable" role="document">
                <div class="modal-content">
                    <div class="modal-header">
                        <h5 class="modal-title">
-                            Audit entry @modalEntry.Id — @modalEntry.EntityType state
+                            Audit entry @modalEntryId@(modalEntry is null ? "" : $" — {modalEntry.EntityType} state")
                        </h5>
                        <button type="button" class="btn-close" @onclick="CloseStateModal" aria-label="Close"></button>
                    </div>
                    <div class="modal-body">
-                        <pre class="bg-body-secondary p-2 rounded small mb-0">@FormatJson(modalEntry.AfterStateJson!)</pre>
+                        @if (modalEntry is null)
+                        {
+                            <div class="alert alert-secondary py-2 mb-0" data-test="modal-entry-gone">
+                                This entry is no longer in the current page of results,
+                                so its state can't be shown.
+                            </div>
+                        }
+                        else
+                        {
+                            <pre class="bg-body-secondary p-2 rounded small mb-0">@FormatJson(modalEntry.AfterStateJson!)</pre>
+                        }
                    </div>
                    <div class="modal-footer">
                        <button type="button" class="btn btn-outline-secondary btn-sm" @onclick="CloseStateModal">Close</button>
@@ -251,23 +251,39 @@
    }
 </div>

-@* Re-resolved from the CURRENT page every render (see DrawerMessage). A row that
-   has been retried, discarded or paged away simply resolves to null and the drawer
-   renders nothing — it can never keep showing a row that no longer exists. *@
+@* Re-resolved from the CURRENT page every render (see DrawerMessage), so the
+   drawer can never keep showing a row that no longer exists.
+
+   The drawer is MOUNTED on _drawerMessageId — user intent — not on the resolve
+   succeeding. Gating the subtree on DrawerMessage would make the drawer's
+   existence a function of list contents, so a render where the row is
+   momentarily absent unmounts it and disposes every event-handler id inside,
+   Close included; a click already in flight against a disposed handler throws
+   GetRequiredEventBindingEntry in the renderer. A row that was retried,
+   discarded or paged away degrades to the notice below instead. *@
@{ var drawer = DrawerMessage; }
-@if (drawer != null)
+@if (_drawerMessageId is not null)
 {
    <div class="offcanvas-backdrop fade show" @onclick="CloseDrawer"></div>
    <div class="offcanvas offcanvas-end show parked-drawer" tabindex="-1" style="visibility: visible;">
        <div class="offcanvas-header border-bottom">
            <div>
                <div class="text-muted small text-uppercase">Parked message</div>
-                <h5 class="offcanvas-title mb-0">@drawer.TargetSystem</h5>
-                <div class="small text-muted">@drawer.MethodName</div>
+                <h5 class="offcanvas-title mb-0">@(drawer?.TargetSystem ?? "No longer listed")</h5>
+                <div class="small text-muted">@drawer?.MethodName</div>
            </div>
            <button type="button" class="btn-close" aria-label="Close" @onclick="CloseDrawer"></button>
        </div>
        <div class="offcanvas-body small">
+        @if (drawer is null)
+        {
+            <div class="alert alert-secondary py-2 mb-0" data-test="drawer-row-gone">
+                This message is no longer on the current page — it was retried,
+                discarded, or the page changed while the drawer was open.
+            </div>
+        }
+        else
+        {
            <dl class="row mb-3">
                <dt class="col-4 text-muted fw-normal">Message ID</dt>
                <dd class="col-8 d-flex align-items-center gap-2">
@@ -304,19 +320,25 @@

            <div class="text-muted text-uppercase small fw-semibold mb-1">Error</div>
            <pre class="bg-body-secondary border rounded p-2 small mb-0 parked-error-pre">@drawer.ErrorMessage</pre>
+        }
        </div>
-        <div class="border-top p-3 d-flex gap-2">
-            <button class="btn btn-outline-success btn-sm flex-grow-1"
-                    @onclick="RetryFromDrawer" disabled="@_actionInProgress">
-                @if (_actionInProgress && _activeAction == "Retry") { <span class="spinner-border spinner-border-sm me-1" role="status"></span> }
-                Retry
-            </button>
-            <button class="btn btn-outline-danger btn-sm flex-grow-1"
-                    @onclick="DiscardFromDrawer" disabled="@_actionInProgress">
-                @if (_actionInProgress && _activeAction == "Discard") { <span class="spinner-border spinner-border-sm me-1" role="status"></span> }
-                Discard
-            </button>
-        </div>
+        @* Actions need the resolved row to act on, so they hide when it is gone.
+           Close stays mounted regardless — see the note above. *@
+        @if (drawer is not null)
+        {
+            <div class="border-top p-3 d-flex gap-2">
+                <button class="btn btn-outline-success btn-sm flex-grow-1"
+                        @onclick="RetryFromDrawer" disabled="@_actionInProgress">
+                    @if (_actionInProgress && _activeAction == "Retry") { <span class="spinner-border spinner-border-sm me-1" role="status"></span> }
+                    Retry
+                </button>
+                <button class="btn btn-outline-danger btn-sm flex-grow-1"
+                        @onclick="DiscardFromDrawer" disabled="@_actionInProgress">
+                    @if (_actionInProgress && _activeAction == "Discard") { <span class="spinner-border spinner-border-sm me-1" role="status"></span> }
+                    Discard
+                </button>
+            </div>
+        }
    </div>
 }

@@ -215,22 +215,44 @@

@* ── Row detail modal ──
   The modal holds only the row's NotificationId and re-resolves the summary from
-   the page currently on screen on every render (DetailRow()). A refresh that
-   replaces the row list therefore never leaves this surface rendering a stale
-   record, and a row that has left the page closes the modal instead of stranding
-   it. *@
-@if (DetailRow() is { } d)
+   the page currently on screen on every render (DetailRow()), so this surface
+   never renders a stale record after a refresh replaces the row list.
+
+   VISIBILITY IS GATED ON _detailNotificationId — user intent — NOT on whether
+   DetailRow() still resolves. That distinction is load-bearing: gating the
+   subtree on the resolve makes the modal's existence a function of list
+   contents, so any render where the row is momentarily absent unmounts the
+   whole subtree and disposes every event-handler id inside it, including
+   Close's. A click already in flight against a disposed handler throws
+   GetRequiredEventBindingEntry in the renderer. Keeping the mount tied to
+   intent means only the user closes this modal; an unresolvable row degrades to
+   the notice below while the frame — and its handlers — stay put. *@
+@if (_detailNotificationId is { } detailId)
 {
+    var d = DetailRow();
    <div class="modal show d-block sb-modal-backdrop" tabindex="-1"
         @onclick="CloseDetail">
        <div class="modal-dialog modal-dialog-scrollable modal-lg" @onclick:stopPropagation="true">
            <div class="modal-content">
                <div class="modal-header">
-                    <h6 class="modal-title">Notification Detail — @ShortId(d.NotificationId)</h6>
+                    <h6 class="modal-title">Notification Detail — @ShortId(detailId)</h6>
                    <button type="button" class="btn-close" aria-label="Close"
                            @onclick="CloseDetail"></button>
                </div>
                <div class="modal-body">
+                @if (d is null)
+                {
+                    @* The row left the current page (refresh, filter or page change)
+                       while the modal was open. The fetched detail below is keyed by
+                       id and stays valid, so only the summary is unavailable. *@
+                    <div class="alert alert-secondary py-2 mb-3" data-test="detail-row-gone">
+                        This notification is no longer in the current page of results,
+                        so its summary can't be shown. The message detail below is
+                        still the one you opened.
+                    </div>
+                }
+                else
+                {
                    <dl class="row mb-0">
                        <dt class="col-sm-3">Notification ID</dt>
                        <dd class="col-sm-9"><code>@d.NotificationId</code></dd>
@@ -280,6 +302,7 @@
                            <dd class="col-sm-9 text-danger">@d.LastError</dd>
                        }
                    </dl>
+                }

                    @* ── Recipients ── *@
                    <hr />
@@ -310,8 +333,9 @@
                        else
                        {
                            <div class="text-muted small">
-                                Not yet resolved — recipients are resolved from list
-                                "@d.ListName" at delivery time.
+                                Not yet resolved — recipients are resolved from
+                                @(d is null ? "the notification's list" : $"list \"{d.ListName}\"")
+                                at delivery time.
                            </div>
                        }
                    }
@@ -340,15 +364,17 @@
                    }
                </div>
                <div class="modal-footer">
-                    @if (d.Status == "Parked")
+                    @* Actions need the resolved row to relay against, so they hide
+                       when it is gone — but Close never does. *@
+                    @if (d is { Status: "Parked" } parked)
                    {
                        <div class="btn-group btn-group-sm" role="group">
                            <button class="btn btn-outline-success"
-                                    @onclick="() => RetryFromDetail(d)" disabled="@_actionInProgress">
+                                    @onclick="() => RetryFromDetail(parked)" disabled="@_actionInProgress">
                                Retry
                            </button>
                            <button class="btn btn-outline-danger"
-                                    @onclick="() => DiscardFromDetail(d)" disabled="@_actionInProgress">
+                                    @onclick="() => DiscardFromDetail(parked)" disabled="@_actionInProgress">
                                Discard
                            </button>
                        </div>
@@ -291,18 +291,25 @@

@* ── Row detail modal ──
   The modal holds only the row's TrackedOperationId and re-resolves the summary
-   from the page currently on screen on every render (DetailRow()). A refresh that
-   replaces the row list therefore never leaves this surface rendering a stale
-   record, and a row that has left the page closes the modal instead of stranding
-   it. *@
-@if (DetailRow() is { } d)
+   from the page currently on screen on every render (DetailRow()), so it never
+   renders a stale record after a refresh replaces the row list.
+
+   Visibility is gated on _detailSiteCallId — user intent — NOT on the resolve
+   succeeding. Gating the subtree on DetailRow() would make the modal's existence
+   a function of list contents, so a render where the row 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 modal. The body reads
+   _detail, which is keyed by id and stays valid either way. *@
+@if (_detailSiteCallId is { } detailId)
 {
+    var d = DetailRow();
    <div class="modal show d-block sb-modal-backdrop" tabindex="-1"
         @onclick="CloseDetail">
        <div class="modal-dialog modal-dialog-scrollable modal-lg" @onclick:stopPropagation="true">
            <div class="modal-content">
                <div class="modal-header">
-                    <h6 class="modal-title">Site Call Detail — @ShortId(d.TrackedOperationId)</h6>
+                    <h6 class="modal-title">Site Call Detail — @ShortId(detailId)</h6>
                    <button type="button" class="btn-close" aria-label="Close"
                            @onclick="CloseDetail"></button>
                </div>
@@ -379,15 +386,17 @@
                    }
                </div>
                <div class="modal-footer">
-                    @if (d.Status == "Parked")
+                    @* Actions need the resolved row to relay against, so they hide
+                       when it is gone — but Close never does. *@
+                    @if (d is { Status: "Parked" } parked)
                    {
                        <div class="btn-group btn-group-sm" role="group">
                            <button class="btn btn-outline-success"
-                                    @onclick="() => RetryFromDetail(d)" disabled="@_actionInProgress">
+                                    @onclick="() => RetryFromDetail(parked)" disabled="@_actionInProgress">
                                Retry
                            </button>
                            <button class="btn btn-outline-danger"
-                                    @onclick="() => DiscardFromDetail(d)" disabled="@_actionInProgress">
+                                    @onclick="() => DiscardFromDetail(parked)" disabled="@_actionInProgress">
                                Discard
                            </button>
                        </div>
@@ -256,6 +256,59 @@ public class NotificationReportDetailModalTests : BunitContext
        });
    }

+    /// <summary>
+    /// The modal must stay mounted when the row it was opened for leaves the
+    /// current page — it is gated on user intent (the held id), never on the
+    /// row still resolving.
+    ///
+    /// This is a race regression guard, not a cosmetic one. When the subtree was
+    /// gated on the resolve, a refresh that dropped the row unmounted the whole
+    /// modal and disposed every event-handler id inside it, Close's included. A
+    /// click already in flight against a disposed handler makes the renderer
+    /// throw GetRequiredEventBindingEntry — which is exactly how this surfaced,
+    /// as an intermittent failure in CloseButton_DismissesModal.
+    /// </summary>
+    [Fact]
+    public void Modal_StaysOpen_WhenItsRowLeavesThePage()
+    {
+        var cut = Render<NotificationReportPage>();
+        cut.WaitForState(() => cut.Markup.Contains("Pump fault at Plant-A"));
+
+        var row = cut.FindAll("tbody tr")
+            .First(r => r.TextContent.Contains("Pump fault at Plant-A"));
+        row.DoubleClick();
+        cut.WaitForState(() => cut.FindAll(".modal.show").Count > 0);
+
+        // The next query drops the opened row, as a refresh, filter change or
+        // page change legitimately can while the modal is open.
+        _queryReply = _queryReply with
+        {
+            Notifications = _queryReply.Notifications!
+                .Where(n => n.NotificationId != "notif-aaaaaaaa-1111-full-id")
+                .ToList(),
+            TotalCount = 1,
+        };
+
+        cut.FindAll("button").First(b => b.TextContent.Contains("Query")).Click();
+        cut.WaitForState(() => !cut.Markup.Contains("Pump fault at Plant-A"));
+
+        // Still mounted, and it says why the summary is missing rather than
+        // vanishing out from under the user.
+        var modal = cut.Find(".modal.show");
+        Assert.NotNull(modal.QuerySelector("[data-test='detail-row-gone']"));
+
+        // The detail that was already fetched is keyed by id, so it survives.
+        Assert.Contains(
+            "Pump-001 tripped on overcurrent at 14:32. Investigate immediately.",
+            modal.TextContent);
+
+        // Row-scoped actions are gone (nothing to relay against) but Close is not,
+        // and it still works — the handler was never disposed.
+        Assert.DoesNotContain("Retry", modal.QuerySelector(".modal-footer")!.TextContent);
+        cut.Find(".modal.show .modal-footer button").Click();
+        cut.WaitForAssertion(() => Assert.Empty(cut.FindAll(".modal.show")));
+    }
+
    protected override void Dispose(bool disposing)
    {
        if (disposing)