lmxopcua/code-reviews/Client.UI/findings.md

# Code Review - Client.UI

| Field | Value |
|---|---|
| Module | `src/Client/ZB.MOM.WW.OtOpcUa.Client.UI` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 0 |

## Checklist coverage

A comprehensive review completes every category, recording "No issues found" where
a category produced nothing rather than leaving it blank.

| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Client.UI-001, Client.UI-002 |
| 2 | OtOpcUa conventions | Client.UI-003, Client.UI-004 |
| 3 | Concurrency & thread safety | Client.UI-005 |
| 4 | Error handling & resilience | Client.UI-006 |
| 5 | Security | Client.UI-007 |
| 6 | Performance & resource management | Client.UI-008 |
| 7 | Design-document adherence | Client.UI-009 |
| 8 | Code organization & conventions | Client.UI-010 |
| 9 | Testing coverage | No issues found |
| 10 | Documentation & comments | Client.UI-011 |

## Findings

### Client.UI-001

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `ViewModels/HistoryViewModel.cs:76`, `ViewModels/HistoryViewModel.cs:77` |
| Status | Resolved |

**Description:** `ReadHistoryAsync` runs as a `RelayCommand` body, which is invoked
on the UI thread, so the bare `IsLoading = true` at line 76 happens to land on the
right thread today. But `Results.Clear()` on the very next line is wrapped in
`_dispatcher.Post(...)`, and the `finally` block also sets `IsLoading` through the
dispatcher (`_dispatcher.Post(() => IsLoading = false)` at line 121). The two
`IsLoading` writes use inconsistent dispatch paths. Because the `Post` in the
`finally` is queued behind the result-population `Post` while the synchronous
line-76 write is not, the loading-indicator updates are not guaranteed to be
ordered relative to the grid population, and the pattern is fragile if the command
is ever invoked off the UI thread (a future caller or test harness).

**Recommendation:** Route the line-76 `IsLoading = true` through `_dispatcher.Post`
for consistency with the rest of the method, or set both `IsLoading` writes
synchronously and only dispatch the `ObservableCollection` mutations.

**Resolution:** Resolved 2026-05-22 — Routed the `IsLoading = true` write through `_dispatcher.Post` to make both `IsLoading` assignments consistent with all other UI state mutations in the method.

### Client.UI-002

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `ViewModels/MainWindowViewModel.cs:255`, `ViewModels/MainWindowViewModel.cs:333` |
| Status | Resolved |

**Description:** `ConnectAsync` calls `await BrowseTree.LoadRootsAsync()` and
`ViewHistoryForSelectedNode` calls `History.SelectedNodeId = ...` by dereferencing
the nullable child view-model properties (`BrowseTreeViewModel?`,
`HistoryViewModel?`) without a null check or `!` operator, while the surrounding
code (lines 258-266) does guard `Subscriptions` and `Alarms` with `!= null`.
`InitializeService()` does assign all five child VMs before these lines run, so a
real NRE is unlikely on the current call path, but the inconsistent guarding masks
intent and the nullable-reference compiler flow analysis cannot prove
`InitializeService()` set the field, so this either produces a CS8602 warning that
is being ignored or relies on warnings being suppressed. A future refactor that
makes `InitializeService()` conditionally skip a VM would introduce a silent crash.

**Recommendation:** Make the guarding consistent: either guard all five child VMs
uniformly, or have `InitializeService()` return non-null references the caller uses
directly so the compiler can prove non-nullness.

**Resolution:** Resolved 2026-05-22 — Added `if (BrowseTree != null)` and `if (History != null)` guards at both dereference sites to match the guarding style already used for `Subscriptions` and `Alarms`.

### Client.UI-003

| Field | Value |
|---|---|
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `ZB.MOM.WW.OtOpcUa.Client.UI.csproj:20-21`, `Program.cs:14-20` |
| Status | Resolved |

**Description:** The csproj references `Serilog` and `Serilog.Sinks.Console`, and
`docs/Client.UI.md` lists Serilog as the logging technology, but no source file in
the module uses Serilog. `Program.BuildAvaloniaApp()` uses Avalonia's
`LogToTrace()` and there is no logger configuration, no log calls, and no rolling
file sink. `CLAUDE.md` mandates "Serilog with rolling daily file sink" as the
logging library preference. The references are dead weight and the documented
logging behaviour does not exist.

**Recommendation:** Either wire up Serilog (a console sink at minimum, ideally the
rolling daily file sink the project standard calls for) and route Avalonia logging
through it, or drop the unused `Serilog` package references and correct
`docs/Client.UI.md`.

**Resolution:** Resolved 2026-05-23 — Honoured the CLAUDE.md mandate by wiring up Serilog with a console sink + a rolling daily file sink (`{LocalAppData}/OtOpcUaClient/logs/client-ui-*.log`, retained 14 days). Added `Serilog.Sinks.File` to the csproj and a `ConfigureLogging()` initializer in `Program.Main` that creates `Log.Logger` before `BuildAvaloniaApp()` and calls `Log.CloseAndFlush()` on exit. Each VM that previously had silent swallow blocks now owns a static `Log.ForContext<>()` logger so failures (subscribe, alarm subscribe, redundancy probe, recursive browse) are written to the rolling file. Avalonia's own logging is still routed through `LogToTrace` — replacing that would require a custom `ILogSink` adapter outside the scope of this finding.

### Client.UI-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `Views/MainWindow.axaml.cs:125-138` |
| Status | Resolved |

**Description:** `OnBrowseCertPathClicked` uses `OpenFolderDialog`, which is
obsolete in Avalonia 11.x (the version pinned in the csproj). The supported
replacement is the `StorageProvider` API
(`StorageProvider.OpenFolderPickerAsync`). Using the obsolete type produces a
compiler obsoletion warning and the API is scheduled for removal in a future
Avalonia major version.

**Recommendation:** Migrate the folder chooser to
`TopLevel.GetTopLevel(this).StorageProvider.OpenFolderPickerAsync(...)`.

**Resolution:** Resolved 2026-05-23 — Replaced `OpenFolderDialog` with `TopLevel.GetTopLevel(this).StorageProvider.OpenFolderPickerAsync(...)`, using `TryGetFolderFromPathAsync(vm.CertificateStorePath)` as the suggested start location and `TryGetLocalPath()` to extract the chosen path. The CS0618 obsoletion warning no longer appears in the build output.

### Client.UI-005

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `ViewModels/MainWindowViewModel.cs:286-304`, `ViewModels/MainWindowViewModel.cs:155-189` |
| Status | Resolved |

**Description:** `SubscriptionsViewModel` and `AlarmsViewModel` attach handlers to
the long-lived `_service` events (`DataChanged`, `AlarmEvent`) in their
constructors and detach them only via `Teardown()`. `Teardown()` is called from
`DisconnectAsync` (operator-initiated disconnect), but it is NOT called from the
`OnConnectionStateChanged` partial method that handles the `Disconnected` state;
that path only calls `Clear()`. When the connection drops server-side (session
lost, network failure) the service raises `ConnectionStateChanged(Disconnected)`
without `DisconnectAsync` ever running, so the alarm/data event handlers remain
attached to a dead service. They are not re-attached on the next connect because
`InitializeService()` early-returns when `_service != null` and the same VM
instances are reused, so there is no handler leak per reconnect, but a late or
buffered `DataChanged`/`AlarmEvent` callback fired during teardown will still mutate
`ObservableCollection`s, and the asymmetry between the two disconnect paths is a
latent correctness hazard.

**Recommendation:** Make the disconnect handling symmetric: call
`Subscriptions?.Teardown()` / `Alarms?.Teardown()` (or otherwise quiesce the event
handlers) from the `Disconnected` branch of the `OnConnectionStateChanged` partial
method, not only from `DisconnectAsync`.

**Resolution:** Resolved 2026-05-22 — Added `Teardown()` calls to the `Disconnected` branch and added `Reattach()` methods (idempotent remove+add) called from the `Connected` branch to restore handlers after a server-side drop + reconnect.

### Client.UI-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `ViewModels/MainWindowViewModel.cs:244-252`, `ViewModels/AlarmsViewModel.cs:88-112`, `ViewModels/SubscriptionsViewModel.cs:79-94` |
| Status | Resolved |

**Description:** Many catch blocks swallow exceptions silently with an empty body
and only a comment (`// Redundancy info not available`, `// Subscribe failed`,
`// Subscription failed; no item added`, and others). When a subscribe,
alarm-subscribe, or redundancy read fails, the operator gets no feedback at all: no
status message, no log entry (compounded by Client.UI-003: there is no logger). A
failed `AddSubscriptionAsync` simply leaves the node un-subscribed with no
indication why. This makes field diagnosis of a misconfigured server or a
permission denial effectively impossible from the UI.

**Recommendation:** Surface failures to the operator: at minimum set a status
message or write the exception to a log. Distinguish "feature not supported"
(condition refresh) from "operation failed" so genuine errors are not hidden.

**Resolution:** Resolved 2026-05-23 — Added an observable `StatusMessage` property on `SubscriptionsViewModel` and `AlarmsViewModel`; each former silent catch now logs through Serilog (via Client.UI-003's logger) and writes a user-visible message. `MainWindowViewModel.InitializeService` subscribes to both child VMs' `StatusMessage` changes and bubbles them up into the shell's `StatusMessage` (which is already bound to the status bar). Soft conditions are distinguished from hard failures: `RequestConditionRefreshAsync` failures log at Information level and surface as "Condition refresh not supported by server" rather than a generic error, matching the recommendation. Redundancy probe failure still leaves `RedundancyInfo` null but now logs at Information level instead of dropping the exception. Regression tests `AddSubscription_OnFailure_SurfacesStatusMessage`, `AddSubscriptionForNodeAsync_OnFailure_SurfacesStatusMessage`, `Subscribe_OnFailure_SurfacesStatusMessage`, and `ConnectCommand_RedundancyFailure_DoesNotBreakConnection` cover the four affected swallow sites.

### Client.UI-007

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Security |
| Location | `Services/UserSettings.cs:22-23`, `Services/JsonSettingsService.cs:38-50`, `ViewModels/MainWindowViewModel.cs:393-408` |
| Status | Resolved |

**Description:** The OPC UA `UserName`-token password is persisted in cleartext.
`UserSettings.Password` is a plain `string`, `JsonSettingsService.Save` serializes
the whole settings object to `settings.json` under `LocalApplicationData`, and
`SaveSettings()` is invoked after every successful connect and on window close. Any
process or user able to read the current user's profile directory can recover the
server credentials. `docs/Client.UI.md` documents that "All connection parameters"
are persisted but does not flag the password among them.

**Recommendation:** Do not persist the password in cleartext. Options: omit it from
the persisted model entirely (re-prompt each launch); encrypt it at rest with
`ProtectedData` (DPAPI) on Windows or an equivalent OS keystore on other platforms;
or store only a non-reversible reference. At minimum, document the cleartext
storage as a known limitation.

**Resolution:** Resolved 2026-05-22 — Removed `Password` from `UserSettings` and stopped writing/reading it in `SaveSettings`/`LoadSettings`; the operator is re-prompted each launch.

### Client.UI-008

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Performance & resource management |
| Location | `ViewModels/MainWindowViewModel.cs:18`, `ViewModels/MainWindowViewModel.cs:125-148`, `App.axaml.cs:18-32` |
| Status | Resolved |

**Description:** `IOpcUaClientService` is declared `IDisposable`
(`IOpcUaClientService.cs:10`), and the concrete service owns an OPC UA session plus
SDK resources. `MainWindowViewModel` holds `_service` for the lifetime of the app
but never calls `_service.Dispose()`: not on window close, not on disconnect, not
anywhere. `DisconnectAsync` calls `DisconnectAsync()` on the service but leaves the
object undisposed, and there is no `IDisposable` implementation on
`MainWindowViewModel` itself. The OPC UA SDK session, certificate validator, and
any background reconnect timers are leaked until process exit. The
`ConnectionStateChanged` handler attached at line 130 is also never detached.

**Recommendation:** Make `MainWindowViewModel` implement `IDisposable`, detach the
`ConnectionStateChanged` handler, and dispose `_service` from `MainWindow.OnClosing`
(alongside the existing `SaveSettings()` call).

**Resolution:** Resolved 2026-05-22 — Added `IDisposable` to `MainWindowViewModel` with a `Dispose()` that detaches `ConnectionStateChanged`, calls `Teardown()` on child VMs, and calls `_service.Dispose()`; wired `Dispose()` into `MainWindow.OnClosing`.

### Client.UI-009

| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | `ViewModels/HistoryViewModel.cs:44-54` |
| Status | Resolved |

**Description:** `HistoryViewModel.AggregateTypes` exposes eight entries: `null`
(Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`.
`docs/Client.UI.md` ("Query Options" table) lists only "Raw (default), Average,
Minimum, Maximum, Count, Start, End" and omits `StandardDeviation`. The doc is
stale relative to the code.

**Recommendation:** Update the "Aggregate" row in `docs/Client.UI.md` to include
Standard Deviation.

**Resolution:** Resolved 2026-05-23 — Added "Standard Deviation" to the Aggregate row of the Query Options table in `docs/Client.UI.md` so it matches the eighth entry already exposed by `HistoryViewModel.AggregateTypes`.

### Client.UI-010

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` |
| Status | Resolved |

**Description:** `DateTimeRangePicker` declares `MinDateTimeProperty` /
`MaxDateTimeProperty` styled properties with public CLR accessors, but neither is
read anywhere in the control. `TryParseDateTime`, `OnStartLostFocus`, and
`OnEndLostFocus` never clamp or reject input against the min/max bounds, and no
XAML binds them. The properties are dead API surface that implies a range
constraint the control does not enforce.

**Recommendation:** Either implement min/max validation in the `LostFocus` parse
path (turn out-of-range input red, as invalid input already is) or remove the two
unused styled properties.

**Resolution:** Resolved 2026-05-23 — Removed `MinDateTimeProperty` / `MaxDateTimeProperty` and their CLR accessors from `DateTimeRangePicker.axaml.cs`. No XAML or external caller binds the properties (grep across the repo confirmed only the control file referenced them), so removing the dead API surface is the correct fix; adding min/max clamping would have been speculative behaviour without a calling site.

### Client.UI-011

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` |
| Status | Resolved |

**Description:** The certificate-store-path `TextBox` watermark reads
`(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208
folder name. Per `CLAUDE.md` / `docs/Client.UI.md` the canonical path is now
`{LocalAppData}/OtOpcUaClient/`, and `ClientStoragePaths` migrates the old
`LmxOpcUaClient/` folder forward. The watermark shows operators an obsolete path
that no longer matches where settings and the PKI store actually live.

**Recommendation:** Update the watermark to reference `OtOpcUaClient/pki`, or bind
it to `ClientStoragePaths.GetPkiPath()` so it cannot drift again.

**Resolution:** Resolved 2026-05-23 — Updated the watermark text in `Views/MainWindow.axaml` from `(default: AppData/LmxOpcUaClient/pki)` to `(default: AppData/OtOpcUaClient/pki)` so it matches the canonical folder name resolved by `ClientStoragePaths` (the binding-to-helper alternative was considered but a static string keeps the watermark cheap; the path is also already documented in `docs/Client.UI.md`).