diff --git a/code-reviews/Client.UI/findings.md b/code-reviews/Client.UI/findings.md index f073792..9d53194 100644 --- a/code-reviews/Client.UI/findings.md +++ b/code-reviews/Client.UI/findings.md @@ -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`). diff --git a/docs/Client.UI.md b/docs/Client.UI.md index 66b1db5..0233d07 100644 --- a/docs/Client.UI.md +++ b/docs/Client.UI.md @@ -198,7 +198,7 @@ All times are in UTC. Invalid input turns red on blur. | Option | Description | |--------|-------------| -| Aggregate | Raw (default), Average, Minimum, Maximum, Count, Start, End | +| Aggregate | Raw (default), Average, Minimum, Maximum, Count, Start, End, Standard Deviation | | Interval (ms) | Processing interval for aggregate queries (shown only for aggregates) | | Max Values | Maximum number of raw values to return (default 1000) | diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Controls/DateTimeRangePicker.axaml.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Controls/DateTimeRangePicker.axaml.cs index cf24f44..39a2473 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Controls/DateTimeRangePicker.axaml.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Controls/DateTimeRangePicker.axaml.cs @@ -30,12 +30,6 @@ public partial class DateTimeRangePicker : UserControl public static readonly StyledProperty EndTextProperty = AvaloniaProperty.Register(nameof(EndText), defaultValue: ""); - public static readonly StyledProperty MinDateTimeProperty = - AvaloniaProperty.Register(nameof(MinDateTime)); - - public static readonly StyledProperty MaxDateTimeProperty = - AvaloniaProperty.Register(nameof(MaxDateTime)); - private bool _isUpdating; public DateTimeRangePicker() @@ -67,18 +61,6 @@ public partial class DateTimeRangePicker : UserControl set => SetValue(EndTextProperty, value); } - public DateTimeOffset? MinDateTime - { - get => GetValue(MinDateTimeProperty); - set => SetValue(MinDateTimeProperty, value); - } - - public DateTimeOffset? MaxDateTime - { - get => GetValue(MaxDateTimeProperty); - set => SetValue(MaxDateTimeProperty, value); - } - protected override void OnLoaded(RoutedEventArgs e) { base.OnLoaded(e); diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Program.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Program.cs index 74960ba..6013f98 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Program.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Program.cs @@ -1,4 +1,6 @@ using Avalonia; +using Serilog; +using ZB.MOM.WW.OtOpcUa.Client.Shared; namespace ZB.MOM.WW.OtOpcUa.Client.UI; @@ -7,8 +9,16 @@ public class Program [STAThread] public static void Main(string[] args) { - BuildAvaloniaApp() - .StartWithClassicDesktopLifetime(args); + ConfigureLogging(); + try + { + BuildAvaloniaApp() + .StartWithClassicDesktopLifetime(args); + } + finally + { + Log.CloseAndFlush(); + } } public static AppBuilder BuildAvaloniaApp() @@ -18,4 +28,35 @@ public class Program .WithInterFont() .LogToTrace(); } + + /// + /// Initializes the Serilog root logger with a console sink + a rolling daily file sink + /// under {LocalAppData}/OtOpcUaClient/logs/. CLAUDE.md mandates Serilog with a + /// rolling daily file sink as the project standard; this is also the only way the swallow + /// blocks in the alarms / subscriptions / redundancy view-models surface a diagnosable + /// trace when an operator hits a problem in the field. + /// + private static void ConfigureLogging() + { + var logsDir = Path.Combine(ClientStoragePaths.GetRoot(), "logs"); + try + { + Directory.CreateDirectory(logsDir); + } + catch + { + // Best-effort; file sink will gracefully fall back if the dir can't be created. + } + + Log.Logger = new LoggerConfiguration() + .MinimumLevel.Information() + .Enrich.FromLogContext() + .WriteTo.Console() + .WriteTo.File( + path: Path.Combine(logsDir, "client-ui-.log"), + rollingInterval: RollingInterval.Day, + retainedFileCountLimit: 14, + shared: true) + .CreateLogger(); + } } \ No newline at end of file diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/AlarmsViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/AlarmsViewModel.cs index d67fd54..db20fad 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/AlarmsViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/AlarmsViewModel.cs @@ -2,6 +2,7 @@ using System.Collections.ObjectModel; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using Opc.Ua; +using Serilog; using ZB.MOM.WW.OtOpcUa.Client.Shared; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; using ZB.MOM.WW.OtOpcUa.Client.UI.Services; @@ -13,9 +14,18 @@ namespace ZB.MOM.WW.OtOpcUa.Client.UI.ViewModels; /// public partial class AlarmsViewModel : ObservableObject { + private static readonly ILogger Logger = Log.ForContext(); + private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientService _service; + /// + /// Last user-visible status message — set when an alarm subscribe / unsubscribe / refresh + /// operation fails so the shell can surface the diagnostic instead of silently dropping it. + /// Genuine failures are distinguished from "feature not supported" (condition refresh). + /// + [ObservableProperty] private string? _statusMessage; + [ObservableProperty] private int _interval = 1000; [ObservableProperty] @@ -95,19 +105,25 @@ public partial class AlarmsViewModel : ObservableObject await _service.SubscribeAlarmsAsync(sourceNodeId, Interval); IsSubscribed = true; + StatusMessage = null; try { await _service.RequestConditionRefreshAsync(); } - catch + catch (Exception refreshEx) { - // Refresh not supported + // Condition refresh is optional on the server side — log at info level and surface + // a soft notice rather than a hard failure so the operator can tell apart "server + // does not advertise refresh" from a genuine subscribe failure. + Logger.Information(refreshEx, "RequestConditionRefresh not supported by server"); + StatusMessage = "Condition refresh not supported by server (subscribed)."; } } - catch + catch (Exception ex) { - // Subscribe failed + Logger.Warning(ex, "SubscribeAlarms failed for {Source}", MonitoredNodeIdText ?? "(all)"); + StatusMessage = $"Subscribe to alarms failed: {ex.Message}"; } } @@ -123,10 +139,12 @@ public partial class AlarmsViewModel : ObservableObject { await _service.UnsubscribeAlarmsAsync(); IsSubscribed = false; + StatusMessage = null; } - catch + catch (Exception ex) { - // Unsubscribe failed + Logger.Warning(ex, "UnsubscribeAlarms failed"); + StatusMessage = $"Unsubscribe alarms failed: {ex.Message}"; } } @@ -136,10 +154,14 @@ public partial class AlarmsViewModel : ObservableObject try { await _service.RequestConditionRefreshAsync(); + StatusMessage = null; } - catch + catch (Exception ex) { - // Refresh failed + // Same as the subscribe-time fallback: refresh is server-side optional. Information- + // level log + soft status so the operator sees why an explicit refresh did nothing. + Logger.Information(ex, "RequestConditionRefresh not supported by server"); + StatusMessage = "Condition refresh not supported by server."; } } @@ -189,19 +211,22 @@ public partial class AlarmsViewModel : ObservableObject await _service.SubscribeAlarmsAsync(nodeId, Interval); IsSubscribed = true; + StatusMessage = null; try { await _service.RequestConditionRefreshAsync(); } - catch + catch (Exception refreshEx) { - // Refresh not supported + Logger.Information(refreshEx, "RequestConditionRefresh not supported by server (restore path)"); + StatusMessage = "Condition refresh not supported by server (restored subscription)."; } } - catch + catch (Exception ex) { - // Subscribe failed + Logger.Warning(ex, "RestoreAlarmSubscription failed for {Source}", sourceNodeId); + StatusMessage = $"Restore alarm subscription failed: {ex.Message}"; } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/MainWindowViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/MainWindowViewModel.cs index d1766a5..d0015e5 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/MainWindowViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/MainWindowViewModel.cs @@ -1,6 +1,7 @@ using System.Collections.ObjectModel; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; +using Serilog; using ZB.MOM.WW.OtOpcUa.Client.Shared; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; using ZB.MOM.WW.OtOpcUa.Client.UI.Services; @@ -12,6 +13,8 @@ namespace ZB.MOM.WW.OtOpcUa.Client.UI.ViewModels; /// public partial class MainWindowViewModel : ObservableObject, IDisposable { + private static readonly ILogger Logger = Log.ForContext(); + private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientServiceFactory _factory; private readonly ISettingsService _settingsService; @@ -137,6 +140,15 @@ public partial class MainWindowViewModel : ObservableObject, IDisposable { if (args.PropertyName == nameof(AlarmsViewModel.ActiveAlarmCount)) _dispatcher.Post(() => ActiveAlarmCount = Alarms.ActiveAlarmCount); + else if (args.PropertyName == nameof(AlarmsViewModel.StatusMessage) + && !string.IsNullOrEmpty(Alarms.StatusMessage)) + _dispatcher.Post(() => StatusMessage = Alarms.StatusMessage!); + }; + Subscriptions.PropertyChanged += (_, args) => + { + if (args.PropertyName == nameof(SubscriptionsViewModel.StatusMessage) + && !string.IsNullOrEmpty(Subscriptions.StatusMessage)) + _dispatcher.Post(() => StatusMessage = Subscriptions.StatusMessage!); }; History = new HistoryViewModel(_service, _dispatcher); @@ -244,15 +256,17 @@ public partial class MainWindowViewModel : ObservableObject, IDisposable SessionLabel = $"{info.ServerName} | Session: {info.SessionName} ({info.SessionId})"; }); - // Load redundancy info + // Load redundancy info — the server may not implement the redundancy facet, in which + // case we leave RedundancyInfo null but log so a field diagnosis can tell the difference + // between "facet not advertised" and "facet errored". The connection itself stays up. try { var redundancy = await _service!.GetRedundancyInfoAsync(); _dispatcher.Post(() => RedundancyInfo = redundancy); } - catch + catch (Exception redundancyEx) { - // Redundancy info not available + Logger.Information(redundancyEx, "GetRedundancyInfo unavailable on this server"); } // Load root nodes diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs index fad17f3..612ed75 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/ViewModels/SubscriptionsViewModel.cs @@ -2,6 +2,7 @@ using System.Collections.ObjectModel; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using Opc.Ua; +using Serilog; using ZB.MOM.WW.OtOpcUa.Client.Shared; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; using ZB.MOM.WW.OtOpcUa.Client.UI.Services; @@ -13,9 +14,17 @@ namespace ZB.MOM.WW.OtOpcUa.Client.UI.ViewModels; /// public partial class SubscriptionsViewModel : ObservableObject { + private static readonly ILogger Logger = Log.ForContext(); + private readonly IUiDispatcher _dispatcher; private readonly IOpcUaClientService _service; + /// + /// Last user-visible status message — set when a subscribe/unsubscribe operation fails so the + /// shell can surface the diagnostic instead of silently dropping the error. Cleared on success. + /// + [ObservableProperty] private string? _statusMessage; + [ObservableProperty] [NotifyCanExecuteChangedFor(nameof(AddSubscriptionCommand))] [NotifyCanExecuteChangedFor(nameof(RemoveSubscriptionCommand))] @@ -85,11 +94,13 @@ public partial class SubscriptionsViewModel : ObservableObject { ActiveSubscriptions.Add(new SubscriptionItemViewModel(nodeIdStr, interval)); SubscriptionCount = ActiveSubscriptions.Count; + StatusMessage = null; }); } - catch + catch (Exception ex) { - // Subscription failed; no item added + Logger.Warning(ex, "AddSubscription failed for {NodeId}", nodeIdStr); + _dispatcher.Post(() => StatusMessage = $"Subscribe failed for {nodeIdStr}: {ex.Message}"); } } @@ -116,9 +127,11 @@ public partial class SubscriptionsViewModel : ObservableObject _dispatcher.Post(() => ActiveSubscriptions.Remove(item)); } - catch + catch (Exception ex) { - // Unsubscribe failed for this item; continue with others + Logger.Warning(ex, "Unsubscribe failed for {NodeId}", item.NodeId); + _dispatcher.Post(() => StatusMessage = $"Unsubscribe failed for {item.NodeId}: {ex.Message}"); + // Continue with the other items in the batch. } } @@ -146,11 +159,13 @@ public partial class SubscriptionsViewModel : ObservableObject { ActiveSubscriptions.Add(new SubscriptionItemViewModel(nodeIdStr, intervalMs)); SubscriptionCount = ActiveSubscriptions.Count; + StatusMessage = null; }); } - catch + catch (Exception ex) { - // Subscription failed + Logger.Warning(ex, "AddSubscriptionForNode failed for {NodeId}", nodeIdStr); + _dispatcher.Post(() => StatusMessage = $"Subscribe failed for {nodeIdStr}: {ex.Message}"); } } @@ -186,9 +201,10 @@ public partial class SubscriptionsViewModel : ObservableObject foreach (var child in children) await AddSubscriptionRecursiveAsync(child.NodeId, child.NodeClass, intervalMs, maxDepth, currentDepth + 1); } - catch + catch (Exception ex) { - // Browse failed for this node; skip it + Logger.Warning(ex, "Recursive browse failed for {NodeId}; skipping subtree", nodeIdStr); + _dispatcher.Post(() => StatusMessage = $"Browse failed for {nodeIdStr}: {ex.Message}"); } } diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Views/MainWindow.axaml b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Views/MainWindow.axaml index f07492e..ec5dc43 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Views/MainWindow.axaml +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.UI/Views/MainWindow.axaml @@ -78,7 +78,7 @@ + Watermark="(default: AppData/OtOpcUaClient/pki)" />