fix(central-ui): resolve CentralUI-006 — push-based deployment status via IDeploymentStatusNotifier, remove 10s polling timer

code-reviews/CentralUI/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 1 |
+| Open findings | 0 |
 
 ## Summary
 
@@ -318,7 +318,7 @@ longer imposes a fixed absolute cap. `dotnet build ScadaLink.slnx` clean;
 |--|--|
 | Severity | Medium |
 | Category | Design-document adherence |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor:196-216` |
 
 **Description**
@@ -341,16 +341,59 @@ If polling is kept as a fallback, fetch only changed/in-progress records.
 
 **Resolution**
 
-_Unresolved — a genuine SignalR-push fix requires an event source in another
-module._ Verified 2026-05-16: `Deployments.razor` does poll every 10s, contrary
-to the design doc. But a real push implementation needs the **Deployment
-Manager** module (`ScadaLink.DeploymentManager` — `DeploymentService` /
-`ArtifactDeploymentService` write the `DeploymentRecord` rows) to raise a
-status-change event/observable that the page subscribes to; there is no such
-event today and no CentralUI-only seam to subscribe to. Building that event
-source is out of scope for a CentralUI-only review. Left Open and surfaced for a
-follow-up that adds a deployment-status broadcaster in DeploymentManager (or a
-design-doc amendment acknowledging the polling fallback).
+Resolved 2026-05-16 (commit `<pending>`) — cross-module fix (CentralUI +
+DeploymentManager), explicitly authorized. Root cause confirmed against the
+source: `Deployments.razor` ran a `Timer` (`OnInitializedAsync` → `StartTimer`,
+10s interval) that, every tick and for every open Blazor circuit, reloaded all
+deployment records (`GetAllDeploymentRecordsAsync`) and the full instance map
+(`GetAllInstancesAsync`) — contradicting Component-CentralUI "Real-Time Updates"
+("transitions push to the UI immediately via SignalR … no polling required").
+
+**Process/DI topology confirmed.** `ScadaLink.Host/Program.cs` calls both
+`AddDeploymentManager()` (line 75) and `AddCentralUI()` (line 77) on the same
+`builder.Services` — DeploymentManager and the Central UI run **in the same
+central Host process**, so a DI singleton is genuinely shared between the
+DeploymentManager services and the Blazor circuit's scoped components. The
+shared-singleton seam is real; no out-of-process fallback was needed.
+
+**What was implemented — push-based updates.** A new
+`IDeploymentStatusNotifier` (`ScadaLink.DeploymentManager/IDeploymentStatusNotifier.cs`)
+with a C# `event Action<DeploymentStatusChange>` and a small payload
+(`DeploymentStatusChange` = deployment id + instance id + new status). Its
+implementation `DeploymentStatusNotifier` invokes each subscriber in isolation
+and swallows/logs handler exceptions so a faulting circuit cannot break the
+deployment pipeline. It is registered as a **singleton** in `AddDeploymentManager`
+(`ServiceCollectionExtensions`). Every place `DeploymentService` writes a
+`DeploymentRecord` status now raises the notifier: the `Pending` create, the
+`InProgress` update, the site-response terminal update, the `Failed` cleanup
+write in the catch block, and the `DeploymentManager-006` reconciled-`Success`
+write — five call sites via a private `NotifyStatusChange` helper.
+`ArtifactDeploymentService` was inspected and writes only
+`SystemArtifactDeploymentRecord` rows, which `Deployments.razor` does not
+display, so it correctly raises nothing. `Deployments.razor` no longer has a
+`Timer`: `OnInitializedAsync` subscribes to `IDeploymentStatusNotifier.StatusChanged`,
+the handler reloads via `InvokeAsync(StateHasChanged)` (the notifier event is
+raised on the DeploymentManager service thread), and `Dispose` unsubscribes.
+Blazor Server pushes the re-render to the browser over its SignalR circuit
+automatically — satisfying the documented design. The existing "Pause/Resume
+updates" toggle now gates whether incoming push events are acted on, and
+"Refresh" still forces a manual reload. CLAUDE.md UI rules kept: Blazor Server +
+Bootstrap, custom components, no third-party frameworks.
+
+Regression tests fail against the pre-fix code and pass after. DeploymentManager
+(`DeploymentStatusNotifierTests`): `DeployInstanceAsync_RaisesStatusChange_ForEveryRecordStatusWrite`
+(pre-fix: no notifier, fails to compile / silent), plus
+`NotifyStatusChanged_WithNoSubscribers_DoesNotThrow` and
+`NotifyStatusChanged_ThrowingSubscriber_DoesNotBreakOtherSubscribers`;
+`ServiceCollectionExtensionsTests.AddDeploymentManager_RegistersDeploymentStatusNotifier_AsSingleton`
+pins the shared-singleton seam. CentralUI (`DeploymentsPushUpdateTests`):
+`Deployments_DoesNotPoll_HasNoRefreshTimer` (pre-fix: the `_refreshTimer` field
+existed — confirmed failing), `Deployments_StatusChange_TriggersReload`, and
+`Deployments_Dispose_UnsubscribesFromNotifier`. `dotnet build ScadaLink.slnx`
+clean (0 warnings); `tests/ScadaLink.DeploymentManager.Tests` 76 passed,
+`tests/ScadaLink.CentralUI.Tests` 257 passed. (`TopologyPageTests`' DI fixture
+was also updated to register the new notifier, since it constructs the real
+`DeploymentService`.)
 
 ### CentralUI-007 — Monitoring nav links to Deployment-only pages are shown to all roles