lmxopcua/docs/reviews/2026-10-21/04-adminui.md

AdminUI review — src/Server/ZB.MOM.WW.OtOpcUa.AdminUI (master @ d51d35bd)

Scope: Blazor Server AdminUI — /raw tree, /uns, equipment page, live-status panels, SignalR bridges + in-process stores, ScriptAnalysis (Roslyn) endpoints + Monaco glue, driver browsers / address pickers, CSV import/export. Production code only. Every finding below was verified by reading the cited code; read-only review, nothing edited.

Overall: this slice is in good shape. The hard Blazor-Server lifecycle problems (timer drain, event unsubscribe, superseded-modal-open races, N+1 queries, Razor string slicing) have clearly been worked through already. What remains is mostly scale limiters (whole-tree renders, per-keystroke Roslyn compiles, unbounded fan-out) and a few stubs still wired into the live UI. One finding is a real correctness hazard (#P1).

---

Memory / Resource leaks

L1. Driver-status / resilience snapshot stores never evict — ghost drivers on /hosts until process restart

• Severity: Medium · Confidence: High
• Location: Hubs/InMemoryDriverStatusSnapshotStore.cs:6-17, Hubs/InMemoryDriverResilienceStatusStore.cs:6-18, consumer Hosts/HostsDriverView.cs (Build iterates snapList, i.e. DriverStore.GetAll()), Components/Pages/Hosts.razor:452-453.
• Evidence:

  private readonly ConcurrentDictionary<string, DriverHealthChanged> _byInstance = new();
  public void Upsert(DriverHealthChanged snapshot) { _byInstance[snapshot.DriverInstanceId] = snapshot; ... }
  public IReadOnlyCollection<DriverHealthChanged> GetAll() => _byInstance.Values.ToArray();
  

  There is no Remove/TTL; HostsDriverView.Build derives the driver rows from the snapshot list, not from DriverInstances.
• Failure scenario: delete (or delete+recreate under a new id) a driver instance; its last DriverHealthChanged stays in the singleton forever. /hosts keeps rendering it as a frozen "stale" row (name/type fall back to id since the config join misses), and HostsDriverView.Build cost grows with every id ever seen since the admin node started. Long-lived admin nodes on a churny fleet accumulate.
• Fix: evict entries whose DriverInstanceId is absent from the config load Hosts already performs every 5 s (_instances), or add a Remove driven by the driver-removal / deploy message, or expire by PublishedUtc (e.g. > 10 min).

L2. DriverTestConnectButton — System.Timers.Timer with an async void Elapsed handler, never drained

• Severity: Low · Confidence: Medium
• Location: Components/Shared/Drivers/DriverTestConnectButton.razor:66-74, 82
• Evidence:

  _clearTimer = new System.Timers.Timer(30_000) { AutoReset = false };
  _clearTimer.Elapsed += async (_, _) => { _result = null; await InvokeAsync(StateHasChanged); };
  ...
  public void Dispose() => _clearTimer?.Dispose();
  

• Failure scenario: Timers.Timer.Dispose does not wait for an in-flight callback, and an async void lambda's post-await exception (component/circuit already gone) cannot be observed by the timer's exception suppression — it surfaces as an unhandled thread-pool exception. The sibling components (DriverStatusPanel, Alerts) explicitly moved to System.Threading.Timer + DisposeAsync for this reason; this one was left behind.
• Fix: mirror DriverStatusPanel.ShowOpResult (System.Threading.Timer, IAsyncDisposable, token check for stale fires).

L3. AlarmsHistorian page — sync timer dispose, no overlap guard, blanket catch {}

• Severity: Low · Confidence: High
• Location: Components/Pages/AlarmsHistorian.razor:58-74, 87
• Evidence: _timer = new Timer(_ => _ = InvokeAsync(RefreshAsync), ...) … public void Dispose() => _timer?.Dispose(); and RefreshAsync wraps the Ask in a bare catch { }.
• Failure scenario: after dispose a queued tick still issues an actor Ask (2 s timeout) against a gone component; every failure (including "this role has no historian actor") is swallowed and the page sits on "Loading…" forever — the comment admits the role-gated message "lands when we add role gating to the UI".
• Fix: copy the Fleet.razor pattern (IAsyncDisposable, _disposed, overlap guard, cancel CTS) and surface the "no historian on this role" state.

L4. Browse sessions can be orphaned when a picker is disposed mid-OpenAsync (bounded by the reaper)

• Severity: Low · Confidence: High
• Location: all Components/Shared/Drivers/Pickers/*AddressPickerBody.razor OpenBrowseAsync (e.g. GalaxyAddressPickerBody.razor:146-157) and RawBrowseModal.razor OpenBrowseAsync; disposal at e.g. GalaxyAddressPickerBody.razor:217-225.
• Evidence: var result = await BrowserService.OpenAsync(...); if (result.Ok) _token = result.Token; — if the hosting modal is closed/navigated away while the open is in flight, DisposeAsync has already run with _token == Guid.Empty, then the token lands on a dead component and nobody calls CloseAsync.
• Failure scenario: an OPC UA client session / MQTT connection / SQL connection stays open against the plant device for up to IdleTtl (2 min) + one reaper tick (30 s). Harmless at low volume; a flapping operator can hold several at once.
• Fix: in OpenBrowseAsync, after the await check a _disposed flag and immediately CloseAsync(result.Token).

L5. In-process stores invoke subscriber delegates synchronously on the bridge actor thread; one throwing subscriber aborts delivery and (for two bridges) restarts the actor

• Severity: Low · Confidence: High
• Location: Hubs/InMemoryDriverStatusSnapshotStore.cs:13, Hubs/IInProcessBroadcaster.cs (Publish => Received?.Invoke(item)), Hubs/AlertSignalRBridge.cs ForwardAsync (the _broadcaster.Publish(msg) sits outside the try), Hubs/DriverResilienceStatusBridge.cs (Receive<…>(msg => _store.Upsert(msg)), no catch).
• Failure scenario: every open circuit's handler runs inline on the actor thread (InvokeAsync is fire-and-forget so it is cheap, but any synchronous throw — e.g. a handler on a circuit whose renderer is mid-teardown — stops the multicast at that subscriber; remaining panels miss the update, and the alert/resilience bridge actor restarts and re-subscribes to DPS). It also means the DPS bridge's throughput is coupled to the number of open admin browser tabs.
• Fix: iterate GetInvocationList() with per-subscriber try/catch (or post to a channel), and move Publish inside the try in AlertSignalRBridge.

---

Unimplemented / stubbed

S1. ctx.SetVirtualTag is advertised by the editor but documented (by the editor itself) as inert at runtime

• Severity: Medium · Confidence: High
• Location: ScriptAnalysis/ScriptAnalysisService.cs:349-353 (hover text), TryGetTagPathLiteral offers path completion for SetVirtualTag (:295-300).
• Evidence: "⚠ Cross-tag SetVirtualTag writes are currently dropped in single-tag mode (the cascade engine is not wired into the host); this write will not take effect at runtime."
• Failure scenario: an author gets completions + green diagnostics for a call that silently does nothing when deployed. Only a hover reveals it.
• Fix: either wire the cascade path or emit an OTSCRIPT_* warning marker from Diagnose for ctx.SetVirtualTag(...) so the Problems list shows it, not just hover.

S2. Inlay-hints endpoint is a no-op, but the JS still POSTs the whole script to it

• Severity: Low · Confidence: High
• Location: ScriptAnalysis/ScriptAnalysisService.cs:468-472 (InlayHints(...) => new(Array.Empty<InlayHint>())), ScriptAnalysisEndpoints.cs:24, wwwroot/js/monaco-init.js:104-121 (registerInlayHintsProvider → fetch("/api/script-analysis/inlay-hints", { body: model.getValue() })).
• Failure scenario: Monaco re-requests inlay hints on content/viewport changes, so every edit costs an extra authenticated POST carrying the full source, for an always-empty answer.
• Fix: drop the provider registration (and endpoint) until the feature exists.

S3. MonacoEditor.MarkersChanged / RevealLineAsync ("problems panel") have no consumer; marker array is still round-tripped JS→.NET after every diagnostics pass

• Severity: Low · Confidence: High
• Location: Components/Shared/MonacoEditor.razor:39-44, 129-131, 134-138 ("The marker DTO is not modelled yet — typed as object[] until a later task…"); monaco-init.js:209 (dotNetRef.invokeMethodAsync("OnMarkersChanged", markers)); grep MarkersChanged finds no consumer in ScriptEdit.razor / VirtualTagModal.razor.
• Fix: either build the problems panel or remove the callback + JS invoke.

S4. Dead service surface: IUnsTreeService.LoadTagsForEquipmentAsync / EquipmentTagRow / tagCounts

• Severity: Low · Confidence: High
• Location: Uns/UnsTreeService.cs (LoadTagsForEquipmentAsync returns Array.Empty<EquipmentTagRow>(); LoadStructureAsync builds an always-empty tagCounts), Uns/EquipmentChildRows.cs:7. No production caller.
• Fix: delete the member, record and the zero-count plumbing (v3 retired the binding).

S5. Deploy API 202 Accepted Location points at a route that does not exist

• Severity: Low · Confidence: High
• Location: Api/DeployApiEndpoints.cs:86-88 — Results.Accepted($"/api/deployments/{r.DeploymentId?.Value}", …); the only route mapped is MapPost("/api/deployments"); no MapGet anywhere in src/Server.
• Failure scenario: a REST client that follows the Location for status polling gets a 404.
• Fix: add a GET that reads Deployments/NodeDeploymentStates for the id, or return Accepted(value) without a location.

S6. Equipment CSV import is a naive comma split (no RFC-4180), unlike the raw-tag CSV import

• Severity: Low · Confidence: High
• Location: Components/Shared/Uns/ImportEquipmentModal.razor ParseCsv — // NOTE: simple comma split — no RFC4180 quoting; values with commas are not supported.
• Failure scenario: a quoted Manufacturer containing a comma silently shifts every later column (Model/SerialNumber…) with no error. The raw-tag importer (CsvParser + CsvColumnMap) already does this correctly.
• Fix: reuse the raw CSV parser.

S7. Dev-only demo page ships in the production assembly

• Severity: Low · Confidence: High
• Location: Components/Pages/Dev/ContextMenuDemo.razor:1-3 — @page "/dev/context-menu-demo", [Authorize(Policy = AuthenticatedRead)].
• Fix: #if DEBUG the route or move it to a test host.

---

Performance limiters

P1. Monaco ships the full editor text JS→.NET on every keystroke; Blazor Server's 32 KB MaximumReceiveMessageSize is never raised

• Severity: High · Confidence: Medium (limit + code path verified; failure mode on exceed is per ASP.NET Core SignalR behaviour)
• Location: wwwroot/js/monaco-init.js:213-214 (editor.onDidChangeModelContent(() => dotNetRef.invokeMethodAsync("OnValueChanged", editor.getValue()).catch(function () {}))), Components/Shared/MonacoEditor.razor:119-127, EndpointRouteBuilderExtensions.cs:56 (AddRazorComponents().AddInteractiveServerComponents() — no AddHubOptions; grep MaximumReceiveMessageSize src → nothing).
• Failure scenario: (a) cost: each keystroke transfers O(script size) over the circuit — a 20 KB script typed at 5 keys/s is 100 KB/s per author, before the separate fetch calls below. (b) correctness: once the script exceeds 32 KB (paste a large script), the JS→.NET invoke exceeds the hub's receive limit; the failure is swallowed by .catch(function () {}), so _lastSentValue/Value silently stop tracking the editor and Save persists stale source (ScriptEdit SubmitAsync reads _form.SourceCode, which only ValueChanged updates). SignalR may also tear the circuit down on the oversize frame.
• Fix: read the text back on demand (MonacoBlazor.getValue already exists) at Save / blur instead of per keystroke, or send onDidChangeModelContent changes and apply them server-side; and raise HubOptions.MaximumReceiveMessageSize (or document the cap).

P2. Roslyn: a full CSharpCompilation + semantic model per request, per keystroke, with no cancellation anywhere

• Severity: Medium · Confidence: High
• Location: ScriptAnalysis/ScriptAnalysisService.cs:60-67 (Analyze → CSharpCompilation.Create(...) every call), callers CompleteAsync:220, Hover:323, SignatureHelp:422, Diagnose:97; ScriptAnalysisEndpoints.cs:19-24 (handlers do not take CancellationToken/RequestAborted); monaco-init.js:32-72 (quickSuggestions.strings = true, providers ignore Monaco's cancellation token, no AbortController).
• Evidence: var compilation = CSharpCompilation.Create("ScriptAnalysis", new[] { tree }, Sandbox.References, CompileOptions); return (tree, compilation, compilation.GetSemanticModel(tree), …).
• Failure scenario: typing inside ctx.GetTag("…") fires a completion request per character plus hover plus signature help plus the 500 ms diagnostics; each is a fresh compile against the full sandbox reference set on the admin node's thread pool, and an abandoned request runs to completion. Two or three concurrent authors on a big script make the admin node (which also hosts the control-plane actors) visibly busy.
• Fix: keep one CSharpCompilation per scoped service and use compilation.ReplaceSyntaxTree (cheap incremental re-bind); plumb HttpContext.RequestAborted through; honour Monaco's CancellationToken + AbortController in the providers; consider server-side debounce on completions.

P3. ScriptTagCatalog.GetPathsAsync rebuilds a HashSet of every fleet tag path and sorts on every completion keystroke

• Severity: Medium · Confidence: High
• Location: ScriptAnalysis/IScriptTagCatalog.cs GetPathsAsync (new HashSet<string>(entries.Select(e => e.Path), OrdinalIgnoreCase) → Where(StartsWith) → OrderBy → Take(200)), GetTagInfoAsync (linear FirstOrDefault per hover).
• Failure scenario: the 5 s snapshot cache fixed the DB side, but the in-memory side is still O(n log n) allocation per keystroke; on a 50k-tag fleet that is tens of MB of garbage per typed character.
• Fix: materialise a sorted, de-duplicated string[] (and a path→info dictionary) inside the Snapshot; prefix-search with Array.BinarySearch.

P4. /hosts rebuilds every cluster group and re-renders the whole page on every driver health publish

• Severity: Medium · Confidence: High
• Location: Components/Pages/Hosts.razor:478-479 (OnSnapshotChanged(DriverHealthChanged _) => InvokeAsync(() => { RebuildDriverGroups(); StateHasChanged(); })), Hosts/HostsDriverView.Build (full re-projection: ToList + per-cluster Where scans), publish cadence documented at Hosts.razor:301-302 ("5 s publish interval").
• Failure scenario: N enabled drivers ⇒ N full rebuilds + N full-page diffs every 5 s per open /hosts tab (200 drivers ≈ 40/s), on top of the page's own 5 s tick. Each HostsDriverRow ctor also re-sorts Diagnostics and HostStatuses.
• Fix: set a dirty flag in OnSnapshotChanged and let the existing 5 s TickAsync do the rebuild (or throttle to ≤1 rebuild/s), and rebuild only the affected cluster.

P5. RawTree re-creates every node's context-menu list per render and re-renders a ContextMenu child per row; no virtualization

• Severity: Medium · Confidence: High
• Location: Components/Shared/Raw/RawTree.razor:107-137 (RenderNode → var items = MenuFor(node) → new List<ContextMenuItem> { new() { … OnClick = () => … } } per node per render), :129 (<ContextMenu Items="items" …> — Items is a reference-type parameter, so Blazor treats it as changed on every render), grep Virtualize → unused anywhere in AdminUI; Uns/RawTreeService.cs LoadDeviceChildrenAsync / LoadTagGroupChildrenAsync return every tag of the device/group.
• Failure scenario: expand a device with a few thousand root tags: every spinner toggle, checkbox click (picker mode ToggleTagSelection calls StateHasChanged), or modal open/close re-renders thousands of rows, allocates thousands of menu lists + closures, and re-renders thousands of ContextMenu children. Same shape in DriverBrowseTree (RenderNode per browse node) and UnsTree.
• Fix: cache the per-node menu on RawNode (invalidate on Enabled change), add ShouldRender/@key discipline, or wrap each expanded level in <Virtualize>; page tags from the service (e.g. 500 per level with a "show more").

P6. Whole CSVs are rendered into the circuit: import review grid has no paging, export embeds the file twice in the DOM

• Severity: Medium · Confidence: High
• Location: Components/Shared/Raw/RawCsvImportModal.razor:85 (@foreach (var row in _result.Rows) — every parsed row of a file up to the 16 MB cap at :261), Components/Shared/Raw/RawCsvExportModal.razor:46 (<textarea …>@_csv</textarea>) and :54 (href="@_dataUri" = base64 of the same text) with both strings retained in _csv/_dataUri while the modal is open.
• Failure scenario: exporting a 50k-tag device produces a multi-MB CSV; the render batch pushed over SignalR is ≈2.3× that size (textarea + base64) and the DOM holds it; importing a large file renders 100k+ <tr>s in one go and recomputes OkCount/ErrorCount/RemainingCount (three full scans) on every render.
• Fix: <Virtualize> the review grid; stream the export via IJSRuntime + DotNetStreamReference or a download endpoint; show a bounded preview (first N lines) in the textarea.

P7. Alerts / ScriptLog: one StateHasChanged per received event; list re-filtered per render

• Severity: Low · Confidence: High
• Location: Components/Pages/ScriptLog.razor:128-134 (OnEntry → InvokeAsync(() => { _rows.Insert(0, entry); …; StateHasChanged(); })), :95-110 (VisibleRows filters + ToList() on every render), Components/Pages/Alerts.razor:279-286.
• Failure scenario: a chatty script (log per evaluation per tag) drives one full-page render per line on every open /script-log tab; Insert(0) on a List<T> of 500 is a memmove per line.
• Fix: batch via a 100–250 ms coalescing timer; use a ring buffer / LinkedList.

P8. /scripts list loads every script with its full SourceCode

• Severity: Low · Confidence: High
• Location: Components/Pages/Scripts.razor:117-123 (db.Scripts.AsNoTracking().OrderBy(s => s.Name).ToListAsync() — full entity), source only needed for the on-demand "view source" dialog (FindSourceScript).
• Fix: project {ScriptId, Name, Language, SourceHash, RowVersion} for the list; fetch SourceCode when the dialog opens.

P9. Diagnostics responses are not sequenced — a slow older response can overwrite newer markers

• Severity: Low · Confidence: High
• Location: wwwroot/js/monaco-init.js:204-212 (scheduleDiagnostics debounces the start but fetchDiagnostics results are applied whenever they arrive).
• Failure scenario: type, pause 500 ms (request A, slow — the admin node is busy per P2), type again, pause (request B, fast): B's markers land, then A's stale markers overwrite them; the editor shows errors for code that no longer exists until the next edit.
• Fix: generation counter captured before the fetch, ignore results whose generation != current; add AbortController.

---

Other hazards

H1. Destructive actions with no confirmation step

• Severity: Medium · Confidence: High
• Location: Components/Pages/Uns/EquipmentPage.razor:243, 298 (per-row Delete buttons bound directly to DeleteVirtualTag / DeleteAlarm, which delete immediately at :447-456, :492-501); Components/Pages/ScriptEdit.razor:72, 145-160 (DeleteAsync deletes the script on click). Contrast: /uns and /raw route every delete through a confirm modal (GlobalUns.razor _confirmNode, RawTree.razor ShowConfirm).
• Failure scenario: a mis-click on a dense row deletes a scripted alarm / virtual tag (and, after the next deploy, its OPC UA node and any ScadaBridge subscription to it) with no undo.
• Fix: reuse RawConfirmDialog.

H2. Modal parameter dictionaries and EventCallbacks are re-created on every render

• Severity: Low · Confidence: High
• Location: Components/Shared/Drivers/DriverConfigModal.razor (_formParameters => new() { … EventCallback.Factory.Create(...) }), DeviceModal.razor (_deviceFormParameters), RawTagModal.razor BuildEditorParameters().
• Failure scenario: DynamicComponent sees new parameter values every render, so the hosted driver/device/tag-config form runs OnParametersSet on every parent render. Correctness is protected only by each form's _lastParsed guard (verified present in all 12 driver forms + 9 device forms); any future form that forgets the guard will clobber in-progress edits on the first live-status push. This is the exact bug class the MqttDriverForm note describes.
• Fix: build the dictionary once per open (in LoadAsync) and cache the callbacks in fields.

H3. monaco.editor.setTheme is global

• Severity: Low · Confidence: High
• Location: monaco-init.js:218-221 (comment acknowledges it) + MonacoEditor.razor:158-162.
• Failure scenario: VirtualTagModal and the host page can each mount an editor; toggling theme on one flips the other, and the per-component _dark flags drift from reality. Cosmetic.

---

Checked-and-fine (looked suspicious, verified OK)

• Timer/event lifecycle on the live pages — Fleet, Hosts, DriverStatusPanel, Alerts, ScriptLog, PageErrorBoundary, Certificates all unsubscribe singleton events, drain System.Threading.Timer via DisposeAsync, cancel a page CTS, and guard StateHasChanged after teardown (_disposed, _tickRunning). ToastNotification binds auto-dismiss delays to a disposal CTS.
• Browse pipeline — BrowseSessionRegistry + BrowseSessionReaper (2 min idle TTL, 30 s tick, drain on shutdown), 20 s per-call timeout, "Browse" buttons hidden while _token != Guid.Empty (no double-open leak), RawBrowseModal.CloseAsync always closes the session, RefreshTree re-keys the tree without opening a second session, rebirth is authorized server-side and re-probed before publish.
• EF query shapes — RawTreeService / UnsTreeService / ScriptTagCatalog / AclScopeService use AsNoTracking + projections and batch child counts with GroupBy/Contains (no N+1 found); rename script scan runs server-side in batches; ImportTagsAsync / ImportEquipmentAsync pre-load lookup sets and SaveChanges once; Deployments pages Take(50), ClusterAudit pages 200.
• Razor string slicing (#504 class) — every [..N] is either guarded by a Length > N check or applied to a fixed-width Guid.ToString("N"); DB values go through DisplayText.Abbreviate.
• Superseded-open races — RawBrowseModal, RawCsvImportModal, RawCsvExportModal, DeviceModal, DriverConfigModal, RawTagModal, VirtualTagModal all carry an open-generation / loaded-key guard so a late await cannot install another target's state.
• Caches — ScriptTagCatalogSnapshotCache (5 s TTL, single-flight gate, keeps old snapshot on failure) and DeploymentDriftProbe (watermark-keyed, single-flight) are bounded single-entry caches, not growing maps. No static mutable state in components (only JsonSerializerOptions).
• Roslyn references — Sandbox.References / Preamble are process-static and shared; no AdhocWorkspace/MetadataReference is created per request (the per-request cost is the compilation itself, see P2).
• JS interop — the single DotNetObjectReference (MonacoEditor) is disposed; MonacoBlazor.dispose is called under JSDisconnectedException handling; prerender InvalidOperationException is handled.
• TelemetryDialSupervisor — the Task.Run dial loop only Tells Self; Dialer.Cancel() cancels and disposes the CTS; reconnect timer per node is replaced, not stacked.
• Auth on the push/analysis surfaces — AlertHub/ScriptLogHub/DriverStatusHub are [Authorize]; /api/script-analysis/* requires ConfigEditor; /api/deployments is API-key gated with constant-time compare.
• Driver-type coverage — all 12 DriverTypeNames have a driver form (DriverFormMaps), tag-config editor + validator; Sql/MTConnect/Calculation deliberately have no device form (SingleConnectionDriverTypes), so nothing is "registered but unauthorable" in this slice.
• GlobalUns "Delete for this node kind is not yet available" — unreachable; UnsTree.RenderActions never renders Delete for Tag/VirtualTag.
• ClusterNamespaces.razor placeholder — intentional redirect stub so old links don't 404.
• InputFile — streamed with a 16 MB cap (not subject to the 32 KB hub limit in P1).
• TwinCATDriverForm "PLC diagnostics… not yet live-gated" — persisted and consumed by the driver; the note is about test coverage, not a dangling option (driver slice).