diff --git a/code-reviews/Client.UI/findings.md b/code-reviews/Client.UI/findings.md index 9d531946..c55ccea3 100644 --- a/code-reviews/Client.UI/findings.md +++ b/code-reviews/Client.UI/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Client/ZB.MOM.WW.OtOpcUa.Client.UI` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `c95a8c6b` | | Status | Reviewed | | Open findings | 0 | @@ -294,3 +294,52 @@ that no longer matches where settings and the PKI store actually live. 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`). + +--- + +## Re-review 2026-06-19 (commit c95a8c6b) + +Re-reviewed all 31 source files at HEAD. All 11 prior findings remain Resolved. Two new findings were recorded for newly introduced code (Shelve/Confirm alarm dialogs, alarm event update handler). + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Client.UI-013 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | Client.UI-012 | +| 10 | Documentation & comments | No issues found | + +### Client.UI-012 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `ViewModels/AlarmsViewModel.cs:60-93` | +| Status | Resolved | + +**Description:** The `OnAlarmEvent` handler has three untested paths: (1) an existing retained alarm receiving an updated event (replace in place), (2) an existing retained alarm becoming non-retained (should be removed), and (3) a non-retained alarm with no existing row (should be silently dropped). Only the "new retained alarm added" path was covered. Without tests for the update and remove paths, the correctness of `AlarmEvents[index] = newItem` and the `ActiveAlarmCount` recalculation after updates could regress silently. + +**Recommendation:** Add tests covering alarm update in place, alarm removal when `Retain` becomes false, non-retained alarm drop, and `ActiveAlarmCount` decrement on acknowledgment state change. + +**Resolution:** Resolved 2026-06-19 — Added four tests to `AlarmsViewModelTests`: `AlarmEvent_ExistingAlarm_UpdatesInPlace`, `AlarmEvent_ExistingAlarmBecomesNonRetained_IsRemovedCleanly`, `AlarmEvent_NonRetained_WithNoExistingRow_IsNotAdded`, and `AlarmEvent_AcknowledgedUpdate_DecrementsActiveAlarmCount`. All four pass (126 total tests green). + +### Client.UI-013 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `ViewModels/AlarmsViewModel.cs:71-82` | +| Status | Resolved | + +**Description:** In `OnAlarmEvent`, when an existing alarm row's `Retain` transitions from `true` to `false`, the prior code created a new `AlarmEventViewModel`, replaced the collection entry at the row's index (firing a `Replace` change notification), and then immediately called `RemoveAt(index)` (firing a second `Remove` notification). The intermediate Replace was unnecessary: the new item was never visible to the user because it was removed on the very next line. The double notification caused the bound `DataGrid` to perform a redundant re-paint and could cause a brief visual flash of the updated-but-about-to-be-removed row. The `AlarmEvent_ExistingAlarmBecomesNonRetained_IsRemovedCleanly` test confirmed `changeCount == 2` against the prior code. + +**Recommendation:** Skip the `Replace` when `!e.Retain`; call `RemoveAt(index)` directly as the single collection mutation. + +**Resolution:** Resolved 2026-06-19 — Restructured `OnAlarmEvent` so the `!e.Retain` branch does only `RemoveAt(index)` (one notification) and the retained-update branch does only `AlarmEvents[index] = new…` (one notification). The regression test `AlarmEvent_ExistingAlarmBecomesNonRetained_IsRemovedCleanly` now passes, confirming exactly one change notification is fired. 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 510ffb7e..ffdaf8fa 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 @@ -71,14 +71,21 @@ public partial class AlarmsViewModel : ObservableObject if (existing != null) { var index = AlarmEvents.IndexOf(existing); - AlarmEvents[index] = new AlarmEventViewModel( - e.SourceName, e.ConditionName, e.Severity, e.Message, - e.Retain, e.ActiveState, e.AckedState, e.Time, - e.EventId, e.ConditionNodeId); - // Remove alarms that are no longer retained if (!e.Retain) + { + // Alarm is no longer retained — remove it with a single collection mutation + // rather than Replace + Remove (which would fire two change notifications). AlarmEvents.RemoveAt(index); + } + else + { + // Alarm is still retained — update the row in place. + AlarmEvents[index] = new AlarmEventViewModel( + e.SourceName, e.ConditionName, e.Severity, e.Message, + e.Retain, e.ActiveState, e.AckedState, e.Time, + e.EventId, e.ConditionNodeId); + } } else if (e.Retain) { diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/AlarmsViewModelTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/AlarmsViewModelTests.cs index 73ac6e10..b47c6f05 100644 --- a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/AlarmsViewModelTests.cs +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.UI.Tests/AlarmsViewModelTests.cs @@ -153,6 +153,101 @@ public class AlarmsViewModelTests _vm.Interval.ShouldBe(1000); } + // --- Alarm update and non-retain remove paths (Client.UI-012 / Client.UI-013) --- + + /// + /// Regression test for Client.UI-013 / Client.UI-012 — a second event for the same + /// source+condition must update the existing row in place rather than adding a duplicate. + /// + [Fact] + public void AlarmEvent_ExistingAlarm_UpdatesInPlace() + { + var first = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Temperature high", + retain: true, activeState: true, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(first); + + var updated = new AlarmEventArgs( + "Source1", "HighAlarm", 750, "Temperature very high", + retain: true, activeState: true, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(updated); + + _vm.AlarmEvents.Count.ShouldBe(1); + _vm.AlarmEvents[0].Severity.ShouldBe((ushort)750); + _vm.AlarmEvents[0].Message.ShouldBe("Temperature very high"); + } + + /// + /// Regression test for Client.UI-013 — when an existing retained alarm becomes + /// non-retained the row must be removed from the collection with exactly ONE collection + /// mutation (not a Replace + Remove pair). + /// + [Fact] + public void AlarmEvent_ExistingAlarmBecomesNonRetained_IsRemovedCleanly() + { + // Seed a retained alarm. + var first = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Active", + retain: true, activeState: true, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(first); + _vm.AlarmEvents.Count.ShouldBe(1); + + // Track collection-change notifications. + var changeCount = 0; + _vm.AlarmEvents.CollectionChanged += (_, _) => changeCount++; + + // Now the alarm is cleared (Retain = false). + var cleared = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Cleared", + retain: false, activeState: false, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(cleared); + + // Row must be removed. + _vm.AlarmEvents.ShouldBeEmpty(); + // Exactly one collection-change notification (Remove), not two (Replace then Remove). + changeCount.ShouldBe(1); + } + + /// + /// Verifies that a non-retained alarm that has no existing row in the collection + /// is silently dropped (not added). + /// + [Fact] + public void AlarmEvent_NonRetained_WithNoExistingRow_IsNotAdded() + { + var nonRetained = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Already cleared", + retain: false, activeState: false, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(nonRetained); + + _vm.AlarmEvents.ShouldBeEmpty(); + } + + /// + /// Verifies that ActiveAlarmCount is updated correctly when an alarm is acknowledged + /// via an event update (AckedState changes from false to true). + /// + [Fact] + public void AlarmEvent_AcknowledgedUpdate_DecrementsActiveAlarmCount() + { + // Seed an unacknowledged active alarm. + var active = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Active", + retain: true, activeState: true, ackedState: false, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(active); + _vm.ActiveAlarmCount.ShouldBe(1); + + // Acknowledge the alarm. + var acked = new AlarmEventArgs( + "Source1", "HighAlarm", 500, "Acked", + retain: true, activeState: true, ackedState: true, time: DateTime.UtcNow); + _service.RaiseAlarmEvent(acked); + + _vm.AlarmEvents.Count.ShouldBe(1); + _vm.AlarmEvents[0].AckedState.ShouldBeTrue(); + _vm.ActiveAlarmCount.ShouldBe(0); + } + /// /// Regression test for Client.UI-006 — when SubscribeAlarmsAsync throws, the failure must be /// surfaced to the operator via the view model's StatusMessage rather than silently swallowed.