From 71f91aa57cf2a9691c9e72a5ab47de148adee35c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 07:29:10 -0400 Subject: [PATCH] =?UTF-8?q?fix(client-ui):=20update=20findings.md=20?= =?UTF-8?q?=E2=80=94=20mark=20Client.UI-001/002/005/007/008=20Resolved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update status and resolution text for the five Medium findings resolved in this batch; lower the Open findings count from 11 to 6. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Client.UI/findings.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/code-reviews/Client.UI/findings.md b/code-reviews/Client.UI/findings.md index b8fdf70..f073792 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 | 11 | +| Open findings | 6 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `ViewModels/HistoryViewModel.cs:76`, `ViewModels/HistoryViewModel.cs:77` | -| Status | Open | +| 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 @@ -53,7 +53,7 @@ is ever invoked off the UI thread (a future caller or test harness). for consistency with the rest of the method, or set both `IsLoading` writes synchronously and only dispatch the `ObservableCollection` mutations. -**Resolution:** _(open)_ +**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 @@ -62,7 +62,7 @@ synchronously and only dispatch the `ObservableCollection` mutations. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `ViewModels/MainWindowViewModel.cs:255`, `ViewModels/MainWindowViewModel.cs:333` | -| Status | Open | +| Status | Resolved | **Description:** `ConnectAsync` calls `await BrowseTree.LoadRootsAsync()` and `ViewHistoryForSelectedNode` calls `History.SelectedNodeId = ...` by dereferencing @@ -80,7 +80,7 @@ makes `InitializeService()` conditionally skip a VM would introduce a silent cra uniformly, or have `InitializeService()` return non-null references the caller uses directly so the compiler can prove non-nullness. -**Resolution:** _(open)_ +**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 @@ -134,7 +134,7 @@ Avalonia major version. | Severity | Medium | | Category | Concurrency & thread safety | | Location | `ViewModels/MainWindowViewModel.cs:286-304`, `ViewModels/MainWindowViewModel.cs:155-189` | -| Status | Open | +| Status | Resolved | **Description:** `SubscriptionsViewModel` and `AlarmsViewModel` attach handlers to the long-lived `_service` events (`DataChanged`, `AlarmEvent`) in their @@ -156,7 +156,7 @@ latent correctness hazard. handlers) from the `Disconnected` branch of the `OnConnectionStateChanged` partial method, not only from `DisconnectAsync`. -**Resolution:** _(open)_ +**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 @@ -189,7 +189,7 @@ message or write the exception to a log. Distinguish "feature not supported" | Severity | Medium | | Category | Security | | Location | `Services/UserSettings.cs:22-23`, `Services/JsonSettingsService.cs:38-50`, `ViewModels/MainWindowViewModel.cs:393-408` | -| Status | Open | +| Status | Resolved | **Description:** The OPC UA `UserName`-token password is persisted in cleartext. `UserSettings.Password` is a plain `string`, `JsonSettingsService.Save` serializes @@ -205,7 +205,7 @@ the persisted model entirely (re-prompt each launch); encrypt it at rest with or store only a non-reversible reference. At minimum, document the cleartext storage as a known limitation. -**Resolution:** _(open)_ +**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 @@ -214,7 +214,7 @@ storage as a known limitation. | Severity | Medium | | Category | Performance & resource management | | Location | `ViewModels/MainWindowViewModel.cs:18`, `ViewModels/MainWindowViewModel.cs:125-148`, `App.axaml.cs:18-32` | -| Status | Open | +| Status | Resolved | **Description:** `IOpcUaClientService` is declared `IDisposable` (`IOpcUaClientService.cs:10`), and the concrete service owns an OPC UA session plus @@ -230,7 +230,7 @@ any background reconnect timers are leaked until process exit. The `ConnectionStateChanged` handler, and dispose `_service` from `MainWindow.OnClosing` (alongside the existing `SaveSettings()` call). -**Resolution:** _(open)_ +**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