Joseph Doherty
9fadead6a6
Add the 7 per-domain design+implementation plans (archreview/plans/) with an index, produced from the 2026-07-08 architecture review. Fix two confirmed doc drifts the review flagged (theme #5): - CLAUDE.md KNOWN LIMITATION 2: the continuous-historization historized-ref feed IS wired (AddressSpaceApplier.FeedHistorizedRefs -> UpdateHistorizedRefs -> recorder); rewrite to reflect that value-capture is code-complete and only the live end-to-end + restart-convergence verification remains. - CLAUDE.md ScriptAnalysis gating: endpoints use Roles=Administrator,Designer via RequireAuthorization, not the FleetAdmin policy.
220 lines
21 KiB
Markdown
220 lines
21 KiB
Markdown
# Architecture Review 04 — AdminUI (Blazor Server)
|
||
|
||
- **Date:** 2026-07-08
|
||
- **Commit:** `9cad9ed0`
|
||
- **Scope:** `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI` (77 razor components, ScriptAnalysis backend, minimal APIs, SignalR bridges, UNS service layer), plus `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests` for coverage assessment. Auth *policy definitions* live in `ZB.MOM.WW.OtOpcUa.Security/ServiceCollectionExtensions.cs` and are referenced where the AdminUI consumes them.
|
||
|
||
---
|
||
|
||
## Architecture Overview
|
||
|
||
### Packaging and mounting
|
||
|
||
The AdminUI is a **Razor Class Library**, not an app: the fused Host calls `AddAdminUI()` / `MapAdminUI<TApp>()` (`EndpointRouteBuilderExtensions.cs:23–63`). All pages render `InteractiveServer`. `Routes.razor` uses `AuthorizeRouteView` (fixing the historical Admin-001 inert-attribute bug) with login redirect + a permission-denied slot. `MainLayout` delegates to the shared `ZB.MOM.WW.Theme` `ThemeShell` chassis.
|
||
|
||
### Page map
|
||
|
||
| Area | Pages |
|
||
|---|---|
|
||
| Dashboards | `Home`, `Fleet` (per-node deploy state, 10 s poll), `Hosts` (Akka topology + driver health, 5 s poll + event push), `AlarmsHistorian` (sink status, 5 s poll) |
|
||
| Live tails | `Alerts` (alarm transitions + DriverOperator Ack/Shelve), `ScriptLog` |
|
||
| Config: clusters | `ClustersList`, `ClusterEdit/Overview/Namespaces/Acls/Audit/Redundancy/Drivers`, `NodeEdit`, `NamespaceEdit`, `AclEdit`, `NewCluster`, 8 driver pages behind `DriverEditRouter` + `DriverTypePicker` |
|
||
| Config: UNS | `GlobalUns` (tree + Area/Line modals + CSV import), `EquipmentPage` (tabbed Details/Tags/VirtualTags/Alarms with `TagModal`/`VirtualTagModal`/`ScriptedAlarmModal`) |
|
||
| Scripting | `Scripts`, `ScriptEdit` (Monaco), `ScriptLog` |
|
||
| Ops/admin | `Deployments`, `Certificates`, `Reservations`, `RoleGrants`, `Account`, `Login` |
|
||
|
||
### API surface
|
||
|
||
- `POST /api/deployments` (`Api/DeployApiEndpoints.cs`) — headless deploy trigger; `AllowAnonymous` but self-gated on `Security:DeployApiKey` via `X-Api-Key` with fixed-time compare, 503 when unconfigured. Clean, well tested.
|
||
- `POST /api/script-analysis/{diagnostics,completions,hover,signature-help,format,inlay-hints}` (`ScriptAnalysis/ScriptAnalysisEndpoints.cs`) — Roslyn-backed Monaco language services, gated `Roles = "Administrator,Designer"`.
|
||
- SignalR hubs `/hubs/{alerts,driverstatus,fleetstatus,scriptlog}` — all `[Authorize]`; retained for out-of-process clients only.
|
||
|
||
### State / broadcast patterns
|
||
|
||
The load-bearing pattern is **DPS topic → per-node bridge actor → in-process singleton → component event subscription**:
|
||
|
||
- `AlertSignalRBridge` / `ScriptLogSignalRBridge` publish to `IInProcessBroadcaster<T>` (stream feeds, with `IsConnected` health for the "live" pill).
|
||
- `DriverStatusSignalRBridge` feeds `IDriverStatusSnapshotStore` (last-value feed).
|
||
- Components subscribe to the singleton's .NET events and marshal via `InvokeAsync`; **no component anywhere opens a server-side `HubConnection` to its own hub** — the known forbidden pattern is documented in `IInProcessBroadcaster.cs:9–15` and honored consistently across all four live surfaces (`Alerts`, `ScriptLog`, `DriverStatusPanel`, `Hosts`).
|
||
|
||
Data access is `IDbContextFactory<OtOpcUaConfigDbContext>` per call. UNS goes through a real service seam (`Uns/UnsTreeService.cs`, 1531 lines, scoped, RowVersion-aware). Driver browse uses a singleton `BrowseSessionRegistry` + `BrowseSessionReaper` (2 min idle TTL, 30 s tick, dispose-outside-dictionary) + per-call 20 s timeouts — a tidy lifecycle design.
|
||
|
||
---
|
||
|
||
## 1. STABILITY
|
||
|
||
### S-1 (Medium) — Timer disposal is inconsistent; three components don't drain in-flight callbacks
|
||
|
||
The codebase's own best practice (stated in comments) is `System.Threading.Timer` + `await DisposeAsync()` so an in-flight tick can't call `StateHasChanged` on a disposed component. `Hosts.razor:348–355`, `Alerts.razor:291–299`, and `DriverStatusPanel.razor:301–310` do this correctly. Three components don't:
|
||
|
||
- `Components/Pages/Fleet.razor:171` — `public void Dispose() => _timer?.Dispose();` (sync, no drain).
|
||
- `Components/Pages/AlarmsHistorian.razor:90` — same.
|
||
- `Components/Shared/Drivers/DriverTestConnectButton.razor:68–81` — uses `System.Timers.Timer` with an `async (_, _)` `Elapsed` handler (line 69; effectively async-void) and a sync `Dispose`, directly contradicting the comment convention in `Alerts.razor:243–244`.
|
||
|
||
**Recommendation:** converge on the `IAsyncDisposable` + `Threading.Timer` idiom in all three; it is already the documented house pattern.
|
||
|
||
### S-2 (Medium) — `Fleet.razor` auto-refresh has no error handling; DB faults become unobserved task exceptions
|
||
|
||
`Fleet.razor:112` schedules `_ = InvokeAsync(LoadAsync)`; `LoadAsync` (`Fleet.razor:119–159`) has `try/finally` but **no catch** — a transient SQL failure faults the discarded task silently and the page shows stale data with no error surface. Contrast `Hosts.razor:269–287`, which catches, logs, and degrades gracefully. `AlarmsHistorian.razor:73–78` has the opposite problem: a bare `catch { }` that leaves the page on "Loading…" forever (the comment admits the role-gating message is a TODO).
|
||
|
||
**Recommendation:** copy the `Hosts.LoadConfigAsync` catch-log-degrade shape into `Fleet.LoadAsync`; give `AlarmsHistorian` an explicit "no historian on this node role" state.
|
||
|
||
### S-3 (Medium) — No `<ErrorBoundary>` anywhere
|
||
|
||
`Routes.razor` / `MainLayout.razor` wrap nothing in `ErrorBoundary`; a single unhandled exception in any event handler or render tears down the entire circuit to the framework error UI. For a 77-component operations console this is a coarse failure mode.
|
||
|
||
**Recommendation:** wrap `@Body` in `MainLayout` in an `ErrorBoundary` with a recover button; consider per-panel boundaries around the live-tail surfaces.
|
||
|
||
### S-4 (Medium) — Delete flows re-fetch RowVersion at click time, defeating the optimistic-concurrency guard
|
||
|
||
`UnsTreeService` implements RowVersion concurrency rigorously (22 `OriginalValue` sites, 10 `DbUpdateConcurrencyException` handlers), and the *edit* paths carry the RowVersion loaded when the modal opened — correct. But the *delete* paths load a **fresh** DTO immediately before deleting and pass its current RowVersion:
|
||
|
||
- `GlobalUns.razor:279–345` (`ConfirmDeleteAsync` — Area/Line/Equipment branches, e.g. line 293).
|
||
- `EquipmentPage.razor:370–380, 415–425, 460–470` (`DeleteTag`/`DeleteVirtualTag`/`DeleteAlarm`).
|
||
|
||
The guard therefore always passes: a concurrent edit between rendering the row and clicking Delete is silently destroyed. The comments ("Load the tag fresh to capture its current RowVersion") show this is deliberate but it makes delete a *last-writer-wins* operation while updates are *first-writer-wins* — an inconsistent contract.
|
||
|
||
**Recommendation:** carry the RowVersion from the rendered row/DTO into the delete call (the list DTOs would need to carry it), or document delete as intentionally unconditional.
|
||
|
||
### S-5 (Medium) — Destructive actions on `EquipmentPage` and `ScriptEdit` have no confirmation
|
||
|
||
`GlobalUns` has a proper delete-confirm modal (`GlobalUns.razor:71–98`) and `DriverStatusPanel` has an inline Restart confirm (`DriverStatusPanel.razor:119–131`). But `EquipmentPage.razor:172/217/263` delete a tag/virtual-tag/alarm on a single click, and `ScriptEdit.razor:140` deletes a script the same way.
|
||
|
||
**Recommendation:** reuse the `GlobalUns` confirm-modal pattern (or a shared `ConfirmButton` component) for all deletes.
|
||
|
||
### S-6 (Low) — `DriverStatusPanel` result-chip timer lacks the stale-fire token guard `Alerts` added
|
||
|
||
`Alerts.razor:244–257` guards against a disposed-but-already-queued timer callback clearing a *newer* result via a token compare. `DriverStatusPanel.razor:287–299` has the identical construct without the guard. Harmless-looking but the race the Alerts comment describes applies equally here.
|
||
|
||
### S-7 (Low) — `AdminOperationsClient.AskAsync` bypasses the 10 s ask timeout
|
||
|
||
`Clients/AdminOperationsClient.cs:58–59` — the generic `AskAsync` (used by `DriverStatusPanel` Reconnect/Restart) forwards only the caller's token; the three typed methods double-guard with `AskTimeout`. Callers do pass a 15 s CTS today, so this is latent, but the seam's contract is inconsistent.
|
||
|
||
### Positive stability notes
|
||
|
||
- The forbidden self-`HubConnection` pattern is absent; every live surface reads the in-process singleton, and the rationale is documented at the seam.
|
||
- Zero `async void` in the project (the one async event-handler lambda is S-1's `System.Timers` case).
|
||
- Event-handler hygiene is excellent: subscribe-before-read TOCTOU ordering (`Alerts.razor:154–159`), unsubscribe-before-timer-drain dispose ordering, capture-then-invoke in `InProcessBroadcaster.SetConnected` (`IInProcessBroadcaster.cs:86–103`).
|
||
- `AdminOperationsActor` access is exclusively via `Ask` through the singleton proxy — no shared mutable state in the AdminUI layer itself; the snapshot store and broadcaster are the only singletons and both are lock/`ConcurrentDictionary`-guarded.
|
||
- `MonacoEditor.razor` disposes cleanly and distinguishes `JSDisconnectedException` (silent) from real `JSException` (logged).
|
||
|
||
---
|
||
|
||
## 2. PERFORMANCE
|
||
|
||
### P-1 (Medium) — Monaco pushes the full document to .NET on every keystroke
|
||
|
||
`wwwroot/js/monaco-init.js:205–208`: `onDidChangeModelContent` → `invokeMethodAsync("OnValueChanged", editor.getValue())` with **no debounce** — the entire script text crosses the SignalR circuit per keystroke, and `ValueChanged.InvokeAsync` re-renders the parent (`ScriptEdit`, virtual-tag modal). Diagnostics are correctly debounced at 500 ms, but the value sync is not.
|
||
|
||
**Recommendation:** debounce the value push (~200–300 ms) or send it only on blur/save/diagnostic-tick; the .NET side only needs the value for Save and the inline problems panel.
|
||
|
||
### P-2 (Medium) — ScriptAnalysis recompiles from scratch on every request, six endpoints deep
|
||
|
||
`ScriptAnalysis/ScriptAnalysisService.cs:60–67`: each call to any endpoint re-parses and creates a fresh `CSharpCompilation`; `Diagnose` additionally runs full `GetDiagnostics()`. A typing user triggers diagnostics (500 ms debounce) + completions + hover + signature-help against the *same text*, each paying a full compile. Static caching of the sandbox references and preamble is already done (good); there is no memoization of `(text → tree/compilation)`.
|
||
|
||
**Recommendation:** add a small LRU (even size-1 keyed on the normalized text) shared by the endpoints; this collapses the common completions-after-diagnostics case. Fine at single-admin scale — flag before multi-user rollout.
|
||
|
||
### P-3 (Low) — Per-circuit DB polling on the dashboard pages
|
||
|
||
`Hosts.razor:229–245` reloads `ClusterNodes` + `DriverInstances` from SQL every 5 s *per open circuit*; `Fleet.razor` runs a `GroupBy` over `NodeDeploymentStates` every 10 s. Driver health itself is event-pushed (good) — only the config enrichment polls. Acceptable today; a shared `IMemoryCache` (or reacting to the deploy event) would decouple cost from viewer count.
|
||
|
||
### P-4 (Low) — `Deployments.razor:98` runs `ConfigComposer.SnapshotAndFlattenAsync` (full config snapshot + hash) on every page load and after every deploy, purely to compute the drift badge. On a large fleet this is the page's dominant cost.
|
||
|
||
### P-5 (Low) — No `<Virtualize>` anywhere; lists are instead bounded by design (`Alerts` cap 200, `Deployments` `Take(50)`, cluster-scoped tables). Adequate. Two nits: `Alerts.razor:63` renders rows without `@key` while prepending (`Insert(0, …)`), forcing whole-table diffs per event ( `EquipmentPage` tables do use `@key`); the `UnsTree` filter re-runs the recursive `VisibleUnder` walk per render (self-documented as bounded/cheap — agreed).
|
||
|
||
### P-6 (Low) — `inlay-hints` round-trips for a documented no-op
|
||
|
||
`monaco-init.js:99–117` registers an inlay-hints provider that POSTs on every model change; `ScriptAnalysisService.InlayHints` (`ScriptAnalysisService.cs:415`) always returns empty. Skip registering the provider until the feature exists.
|
||
|
||
---
|
||
|
||
## 3. CONVENTIONS
|
||
|
||
### C-1 (High) — Three authorization idioms coexist, and the largest mutating surface is effectively ungated
|
||
|
||
Observed gates:
|
||
|
||
| Idiom | Where |
|
||
|---|---|
|
||
| `Roles = "Administrator,Designer"` | `Deployments.razor:12`, `Scripts.razor`, `ScriptEdit.razor:5`, `ScriptAnalysisEndpoints.cs:18` |
|
||
| `Policy = "FleetAdmin"` | `RoleGrants.razor:2` (page), `Certificates.razor:90,186` (per-action) |
|
||
| `Policy "DriverOperator"` (imperative) | `Alerts.razor:165`, `DriverStatusPanel.razor:166` |
|
||
| **bare `[Authorize]`** | **everything else** — `GlobalUns`, `EquipmentPage`, `ClusterEdit`, `NodeEdit`, `NamespaceEdit`, `AclEdit`, all 8 driver pages, `Reservations` |
|
||
|
||
Consequence: any authenticated user — including a read-only viewer — can create/edit/delete areas, lines, equipment, tags, virtual tags, scripted alarms, driver instances, ACLs, and namespaces; only the *deploy* button is role-gated. Project memory says global-roles/simplest-authz is a deliberate posture, but the current state isn't "simplest" — it's three idioms plus an unguarded majority, which is exactly the drift that produces surprises later.
|
||
|
||
**Recommendation:** centralize policy names as constants (they exist only as string literals today), pick policies over role strings (`FleetAdmin` already means `RequireRole("Administrator")`), and apply one write-gate (`Administrator,Designer` equivalent) to every config-mutating page. This is a one-attribute-per-page change.
|
||
|
||
### C-2 (Medium) — CLAUDE.md / code drift on the ScriptAnalysis gate
|
||
|
||
CLAUDE.md states the `/api/script-analysis/*` endpoints are "gated by the `FleetAdmin` policy"; the code uses `Roles = "Administrator,Designer"` (`ScriptAnalysisEndpoints.cs:17–18`, comment says it deliberately matches the Scripts page). Fix the doc (or converge per C-1).
|
||
|
||
### C-3 (Good) — Driver-typed tag-editor pattern is complete and consistent
|
||
|
||
`Uns/TagEditors/TagConfigEditorMap.cs:10–20` and `TagConfigValidator.cs:12–22` cover **all seven** non-Galaxy drivers (Modbus, S7, AbCip, AbLegacy, TwinCat, Focas, OpcUaClient) with parallel keys; Galaxy intentionally uses the live-browse picker + raw `{"FullName": …}` JSON (`TagModal.razor:96–125`). Every editor follows the same shell-over-pure-model shape (`FromJson`/`ToJson`/`Validate`, preserve-unknown-keys), with the parse-only-on-real-change guard against parent re-renders (`ModbusTagConfigEditor.razor:52–57`). The composable root-key seams (`TagHistorizeConfig`, `TagArrayConfig`, `NativeAlarmModel`) merge onto the same JSON without clobbering — a genuinely good design.
|
||
|
||
### C-4 (Good) — The numeric-enum serialization bug class is closed
|
||
|
||
All 8 driver pages carry `JsonStringEnumConverter` in their serializer options; tag editors bind enums as strings via `Enum.GetValues`/`TryParse`; and the fix is pinned by per-driver `*FormSerializationTests` plus `DriverPageJsonConverterTests` in the test project — regression-proofed, not just patched.
|
||
|
||
### C-5 (Medium) — Two data-access idioms: service seam vs. EF-in-page
|
||
|
||
UNS pages go through `IUnsTreeService` (testable, RowVersion-aware, 12 test files). But `ScriptEdit.razor:87–156`, `Scripts`, `Deployments.razor:83–100`, `Fleet`, and `Hosts` inject `IDbContextFactory` and run EF (including entity mutation and `SaveChangesAsync`) directly in `@code`. The service-seam pages are the ones with meaningful test coverage; the EF-in-page logic is untestable without a host. New pages copy whichever file is nearest.
|
||
|
||
**Recommendation:** declare the service-seam the convention; migrate `ScriptEdit`'s save/delete into the existing `UnsTreeService` script methods (`UpdateScriptSourceAsync` already exists at `IUnsTreeService.cs:502`).
|
||
|
||
### C-6 (Low) — Naming/marker nits
|
||
|
||
`"GalaxyMxGateway"` is a bare string literal in `TagModal.razor:311`; the `DataTypes` array (`TagModal.razor:244`) duplicates type lists that exist elsewhere; casing drifts across `TwinCat` (map key) / `TwinCATTagConfigEditor` / `FOCASAddressPickerBody` / `FocasTagConfigEditor`. CSS is disciplined (one `site.css` + shared theme kit; chip/panel classes reused consistently, `chip-warn` vs `chip-caution` both exist — verify both are defined in the theme).
|
||
|
||
---
|
||
|
||
## 4. UNDERDEVELOPED AREAS
|
||
|
||
### U-1 — Test coverage: strong C# seams, zero component rendering coverage
|
||
|
||
443 `[Fact]`/`[Theory]` cases covering: all tag-config models + validator, `UnsTreeService` (structure/tags/vtags/import/equip-token/script CRUD against a test DB), browse session service/registry/reaper, `CertificateStoreManager`, audit writers, `DeployApiEndpoints` auth matrix, driver-page form serialization (the enum bug class), 8 ScriptAnalysis suites (diagnose/completions/hover/format/tag-path/equip-token), `InProcessBroadcaster`, `HostsDriverView`, address builders. That is a deliberately shaped portfolio around the pure seams.
|
||
|
||
There is **no bUnit** — 77 razor components have no render/binding coverage, which is the documented repo posture ("no bUnit — live-verify"; the `@`-binding bug class passes all unit tests). The consequence is that everything in `@code` blocks that isn't extracted (e.g. all of `EquipmentPage`'s tab/delete handlers, `GlobalUns`'s modal orchestration, `Alerts`' row-action state machine) is verified only by manual `/run` sessions.
|
||
|
||
**Recommendation:** either adopt bUnit for the modal/table state machines, or keep extracting page logic into plain classes (the `HostsDriverView`/`VirtualTagModalHelpers` precedent) so the untested residue in `@code` stays trivial.
|
||
|
||
### U-2 — Stub/known-gap behaviors
|
||
|
||
- `InlayHints` is a wired no-op endpoint + active client provider (P-6); documented in `ScriptAnalysisService.cs:22–25`.
|
||
- `AlarmsHistorian.razor:73–78` — admits the missing role-gated "no historian on this node" message; page silently shows "Loading…" forever on admin-only nodes.
|
||
- `MonacoEditor.razor:33–37` — `MarkersChanged` is typed `object[]` with a comment that the DTO "is not modelled yet".
|
||
- `GlobalUns.ConfirmDeleteAsync` default branch returns "Delete for this node kind is not yet available." for Tag/VirtualTag nodes (`GlobalUns.razor:~320`) — consistent with equipment-page ownership, but the message leaks a not-implemented posture into the UI.
|
||
|
||
### U-3 — Raw-JSON fallback surfaces
|
||
|
||
`TagModal.razor:130–136` falls back to a schemaless textarea for unmapped drivers ("Validated server-side at deploy" — i.e., no client validation), and the Galaxy path allows direct JSON editing beside the picker. With all current drivers mapped (C-3), the fallback today only serves Galaxy + future drivers, but it remains the escape hatch through which invalid configs reach deploy time. `_form.TagConfig` is `[Required]` yet never checked for *well-formed JSON* in the fallback path.
|
||
|
||
**Recommendation:** at minimum, `JsonDocument.Parse` the fallback textarea in `SaveAsync` before submitting.
|
||
|
||
### U-4 — Accessibility
|
||
|
||
~31 `aria-*`/`role=` attributes across 77 components, concentrated in the modals (`role="dialog"`, `btn-close` aria-labels). Gaps: text-glyph expander buttons (`▼`/`▶` in `UnsTree.razor:49`) with no `aria-expanded`/label; modals have no focus trap or Escape handling; live tails (`Alerts`) have no `aria-live` region; tables lack captions/scope. Form labeling is decent (most inputs have `for`/`id` pairs). Acceptable for an internal ops tool; worth a targeted pass on the tree and the modals if operator diversity matters.
|
||
|
||
---
|
||
|
||
## Maturity Ratings
|
||
|
||
| Dimension | Rating (1–5) | Justification |
|
||
|---|---|---|
|
||
| Stability | **4** | Disciplined disposal/marshaling with documented race reasoning and the self-HubConnection ban honored everywhere; docked for three non-draining timers, `Fleet`'s uncaught refresh path, no `ErrorBoundary`, and the RowVersion-refetch delete pattern. |
|
||
| Performance | **3** | Event-push where it matters and bounded lists, but keystroke-grain Monaco interop, per-request Roslyn recompiles ×6 endpoints, per-circuit DB polling, and a full config snapshot per Deployments render. |
|
||
| Conventions | **3** | The tag-editor/validator pattern is exemplary and test-pinned, but authorization is a three-idiom mix with the main mutating surface ungated, plus service-seam vs EF-in-page duality and a CLAUDE.md gate-description drift. |
|
||
| Underdeveloped | **3** | 443 well-aimed seam tests, but zero rendering coverage for 77 components (by policy), several admitted stubs, an unvalidated raw-JSON escape hatch, and thin accessibility. |
|
||
|
||
---
|
||
|
||
## Top Recommendations (ordered)
|
||
|
||
1. **C-1:** Centralize policy constants and put a write-role gate on every config-mutating page (`/uns`, equipment, cluster/driver/ACL/namespace editors, Reservations).
|
||
2. **S-4 + S-5:** Make deletes carry the rendered RowVersion (or document last-writer-wins) and add confirm dialogs on `EquipmentPage`/`ScriptEdit` deletes.
|
||
3. **S-3:** Add an `ErrorBoundary` around `@Body`.
|
||
4. **P-1:** Debounce Monaco's value push to .NET.
|
||
5. **S-1/S-2:** Converge the three straggler timers on the async-dispose idiom and give `Fleet.LoadAsync` the `Hosts`-style catch.
|
||
6. **C-5/U-1:** Declare the service-seam pattern canonical and keep extracting `@code` logic into testable classes.
|