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

fix(client-ui): update findings.md — mark Client.UI-001/002/005/007/008 Resolved

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 07:29:10 -04:00
parent c7f8a00635
commit 71f91aa57c
1 changed files with 11 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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