From 2b33b64a583f08d9b9b3e6bafc24764497411944 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 23 May 2026 07:24:07 -0400 Subject: [PATCH] fix(admin): resolve Low code-review findings (Admin-010,011,012) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Admin-010: vendor Bootstrap 5.3.3 (CSS + JS bundle + maps + provenance README) under wwwroot/lib/bootstrap and reference local paths from App.razor — Admin no longer pulls Bootstrap from jsDelivr. - Admin-011: swap FleetStatusPoller's three plain dictionaries for ConcurrentDictionary so ResetCache can't race a poll tick. - Admin-012: drop the EquipmentId column from EquipmentCsvImporter (per admin-ui.md — equipment id is system-derived from EquipmentUuid); EquipmentImportBatchService and the textarea placeholder updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Admin/findings.md | 14 +-- .../Components/App.razor | 6 +- .../Pages/Clusters/ImportEquipment.razor | 2 +- .../Hubs/FleetStatusPoller.cs | Bin 8440 -> 8785 bytes .../Services/EquipmentCsvImporter.cs | 18 +-- .../Services/EquipmentImportBatchService.cs | 18 ++- .../wwwroot/lib/bootstrap/README.md | 16 +++ .../lib/bootstrap/css/bootstrap.min.css | 6 + .../lib/bootstrap/css/bootstrap.min.css.map | 1 + .../lib/bootstrap/js/bootstrap.bundle.min.js | 7 ++ .../bootstrap/js/bootstrap.bundle.min.js.map | 1 + .../BootstrapVendoringTests.cs | 64 ++++++++++ .../EquipmentCsvImporterTests.cs | 38 +++--- .../EquipmentCsvNoEquipmentIdColumnTests.cs | 74 +++++++++++ .../EquipmentImportBatchServiceTests.cs | 23 ++-- .../FleetStatusPollerConcurrencyTests.cs | 115 ++++++++++++++++++ 16 files changed, 355 insertions(+), 48 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/README.md create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/css/bootstrap.min.css create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/css/bootstrap.min.css.map create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/js/bootstrap.bundle.min.js create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/js/bootstrap.bundle.min.js.map create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/BootstrapVendoringTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/EquipmentCsvNoEquipmentIdColumnTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/FleetStatusPollerConcurrencyTests.cs diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 5ed20d2..3732f74 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -168,13 +168,13 @@ | Severity | Low | | Category | OtOpcUa conventions | | Location | `Components/App.razor:9,16` | -| Status | Open | +| Status | Resolved | **Description:** `App.razor` loads Bootstrap CSS and JS from the `cdn.jsdelivr.net` CDN. `admin-ui.md` section "Tech Stack" specifies "Bootstrap 5 vendored under `wwwroot/lib/bootstrap/`" precisely so the Admin app has no third-party runtime dependency. A CDN reference makes the UI fail in air-gapped / locked-down fleet deployments (a stated deployment target), introduces an uncontrolled third-party origin, and is not covered by a Subresource Integrity hash. **Recommendation:** Vendor Bootstrap under `wwwroot/lib/bootstrap/` and reference the local copies, as the design doc requires. If a CDN is retained for any asset, add `integrity` + `crossorigin` SRI attributes. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — Bootstrap 5.3.3 (CSS + JS bundle, plus their source maps) vendored under `src/Server/ZB.MOM.WW.OtOpcUa.Admin/wwwroot/lib/bootstrap/{css,js}/`; `App.razor` now references the local copies (`lib/bootstrap/css/bootstrap.min.css`, `lib/bootstrap/js/bootstrap.bundle.min.js`); a README under the vendor directory records provenance + upgrade steps. Covered by `BootstrapVendoringTests` (asserts no `cdn.jsdelivr.net`/`cdnjs`/`unpkg` references in `App.razor`, that the vendored files exist with non-trivial sizes, and that `App.razor` references the vendored paths) — verified failing pre-fix, passing post-fix. ### Admin-011 @@ -183,13 +183,13 @@ | Severity | Low | | Category | Concurrency & thread safety | | Location | `Hubs/FleetStatusPoller.cs:24-26,98-103` | -| Status | Open | +| Status | Resolved | **Description:** `FleetStatusPoller` keeps three plain `Dictionary<>` fields (`_last`, `_lastRole`, `_lastResilience`) mutated from `PollOnceAsync`. The poller `ExecuteAsync` loop is single-threaded so the steady-state poll path is safe, but `ResetCache()` (exposed `internal` for tests) clears those same dictionaries with no synchronization. If a test (or any caller) invokes `ResetCache()` while a poll tick is mid-iteration, the `Dictionary` enumeration/mutation race can throw `InvalidOperationException` or corrupt state. **Recommendation:** Either document `ResetCache()` as "only safe when the poller is stopped" and have tests stop the service first, or guard the three dictionaries with a lock / swap them atomically. Using `ConcurrentDictionary` (as the sibling `ResilientLdapGroupRoleMappingService` does) would make the intent explicit. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `_last`, `_lastRole`, and `_lastResilience` swapped from plain `Dictionary<,>` to `ConcurrentDictionary<,>` so concurrent `ResetCache()` / poll-tick mutations are safe by construction (the recommendation's "explicit intent" form). Covered by `FleetStatusPollerConcurrencyTests` — one test guards the structural choice via reflection so a future refactor cannot silently revert; the other stress-runs concurrent mutate + `ResetCache()` via reflection, verifying the race throws no exception (verified failing pre-fix with `Dictionary<,>`). ### Admin-012 @@ -198,13 +198,13 @@ | Severity | Low | | Category | Design-document adherence | | Location | `Services/EquipmentCsvImporter.cs:18-19,33-37,229,232` | -| Status | Open | +| Status | Resolved | **Description:** `EquipmentCsvImporter` declares `EquipmentId` as a required CSV column and parses it into a `required` field. `admin-ui.md` section "Equipment CSV import" (revised after adversarial review finding #4) is explicit: "No `EquipmentId` column — operator-supplied EquipmentId would mint duplicate equipment identity on typos ... never accepted from CSV imports." `EquipmentId` is system-derived (`EQ-` plus first 12 hex chars of `EquipmentUuid`). Accepting it from CSV either contradicts the design or silently lets an import set an identity field the doc says is un-settable. The XML doc on the class also cites the column as required per "decision #117", so either the code or the design doc is stale. `EquipmentImportBatchService.StageRowsAsync` propagates `row.EquipmentId` into the staging row, so any change must cover the finalize path. **Recommendation:** Reconcile with the design: drop `EquipmentId` from `RequiredColumns` and the `EquipmentCsvRow` shape (deriving it from `EquipmentUuid` at finalize time), or — if accepting it is a deliberate reversal — update `admin-ui.md` and the decision log so the two agree. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — code reconciled with the design: `EquipmentId` dropped from `EquipmentCsvImporter.RequiredColumns`, `BuildRow`, `GetCell`, and the `EquipmentCsvRow` shape; the class XML doc now records the admin-ui.md "No EquipmentId column" rule. The finalize path is covered: `EquipmentImportBatchService.StageRowsAsync` now derives the staging-row's `EquipmentId` via `DraftValidator.DeriveEquipmentId(equipmentUuid)`, and `FinaliseBatchAsync` re-derives it from the UUID that actually lands in the `Equipment` row (so a blank/invalid staged UUID that gets replaced by `Guid.NewGuid()` no longer leaves `EquipmentId` and `EquipmentUuid` out of sync). `ImportEquipment.razor`'s textarea placeholder updated to the new header shape. Covered by `EquipmentCsvNoEquipmentIdColumnTests` (five tests guarding `RequiredColumns`/`OptionalColumns`/`EquipmentCsvRow` shape and asserting CSVs with an `EquipmentId` column are rejected as unknown while CSVs without are accepted) — verified failing pre-fix, passing post-fix. The existing `EquipmentCsvImporterTests` + `EquipmentImportBatchServiceTests` were updated to the new header shape and pass green (DB-backed suite ran against `10.100.0.35,14330`). ### Admin-013 diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/App.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/App.razor index b77ac80..e1fb24e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/App.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/App.razor @@ -6,14 +6,16 @@ OtOpcUa Admin - + @* Admin-010: Bootstrap 5 is vendored under wwwroot/lib/bootstrap/ per admin-ui.md + "Tech Stack" — no public-CDN dependency so air-gapped fleet deployments work. *@ + - + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ImportEquipment.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ImportEquipment.razor index d3bf9c4..e640c8e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ImportEquipment.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ImportEquipment.razor @@ -79,7 +79,7 @@