dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(client-ui): resolve Low code-review findings (Client.UI-003,004,006,009,010,011)

Browse Source 
- Client.UI-003: wire Serilog properly per CLAUDE.md — console sink +
  rolling daily file sink in Program.Main, Log.CloseAndFlush in finally,
  per-VM Log.ForContext<> loggers.
- Client.UI-004: migrate the cert-store folder picker from the obsolete
  OpenFolderDialog to StorageProvider.OpenFolderPickerAsync (with
  TryGetFolderFromPathAsync seed + TryGetLocalPath extraction).
- Client.UI-006: surface formerly silent catch blocks via an observable
  StatusMessage on the Subscriptions / Alarms VMs that bubbles up into
  the shell's status bar; soft fallbacks log at Information level so
  hard failures stay distinguishable.
- Client.UI-009: docs/Client.UI.md now lists Standard Deviation in the
  Aggregate row of the Query Options table.
- Client.UI-010: removed the unused MinDateTimeProperty /
  MaxDateTimeProperty styled properties from DateTimeRangePicker.
- Client.UI-011: updated the cert-store TextBox watermark from the
  legacy AppData/LmxOpcUaClient/pki to the canonical
  AppData/OtOpcUaClient/pki.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 11:25:20 -04:00
parent 59ecd18169
commit 1b10194634
14 changed files with 246 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
code-reviews/Client.UI/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 6 |
| Open findings | 0 |
## Checklist coverage
@@ -89,7 +89,7 @@ directly so the compiler can prove non-nullness.
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `ZB.MOM.WW.OtOpcUa.Client.UI.csproj:20-21`, `Program.cs:14-20` |
| Status | Open |
| 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
@@ -104,7 +104,7 @@ rolling daily file sink the project standard calls for) and route Avalonia loggi
through it, or drop the unused `Serilog` package references and correct
`docs/Client.UI.md`.
**Resolution:** _(open)_
**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
@@ -113,7 +113,7 @@ through it, or drop the unused `Serilog` package references and correct
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `Views/MainWindow.axaml.cs:125-138` |
| Status | Open |
| Status | Resolved |
**Description:** `OnBrowseCertPathClicked` uses `OpenFolderDialog`, which is
obsolete in Avalonia 11.x (the version pinned in the csproj). The supported
@@ -125,7 +125,7 @@ Avalonia major version.
**Recommendation:** Migrate the folder chooser to
`TopLevel.GetTopLevel(this).StorageProvider.OpenFolderPickerAsync(...)`.
**Resolution:** _(open)_
**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
@@ -165,7 +165,7 @@ method, not only from `DisconnectAsync`.
| Severity | Low |
| Category | Error handling & resilience |
| Location | `ViewModels/MainWindowViewModel.cs:244-252`, `ViewModels/AlarmsViewModel.cs:88-112`, `ViewModels/SubscriptionsViewModel.cs:79-94` |
| Status | Open |
| Status | Resolved |
**Description:** Many catch blocks swallow exceptions silently with an empty body
and only a comment (`// Redundancy info not available`, `// Subscribe failed`,
@@ -180,7 +180,7 @@ permission denial effectively impossible from the UI.
message or write the exception to a log. Distinguish "feature not supported"
(condition refresh) from "operation failed" so genuine errors are not hidden.
**Resolution:** _(open)_
**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
@@ -239,7 +239,7 @@ any background reconnect timers are leaked until process exit. The
| Severity | Low |
| Category | Design-document adherence |
| Location | `ViewModels/HistoryViewModel.cs:44-54` |
| Status | Open |
| Status | Resolved |
**Description:** `HistoryViewModel.AggregateTypes` exposes eight entries: `null`
(Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`.
@@ -250,7 +250,7 @@ stale relative to the code.
**Recommendation:** Update the "Aggregate" row in `docs/Client.UI.md` to include
Standard Deviation.
**Resolution:** _(open)_
**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
@@ -259,7 +259,7 @@ Standard Deviation.
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` |
| Status | Open |
| Status | Resolved |
**Description:** `DateTimeRangePicker` declares `MinDateTimeProperty` /
`MaxDateTimeProperty` styled properties with public CLR accessors, but neither is
@@ -272,7 +272,7 @@ constraint the control does not enforce.
path (turn out-of-range input red, as invalid input already is) or remove the two
unused styled properties.
**Resolution:** _(open)_
**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
@@ -281,7 +281,7 @@ unused styled properties.
| Severity | Low |
| Category | Documentation & comments |
| Location | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` |
| Status | Open |
| Status | Resolved |
**Description:** The certificate-store-path `TextBox` watermark reads
`(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208
@@ -293,4 +293,4 @@ 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:** _(open)_
**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`).