From 5bf2f2caf1b1d364b946ac8b0326ac810f2d65ba Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:56:33 -0500 Subject: [PATCH] fix(theme): nav-state.js tracks wired sections in a WeakSet so enhanced navigation cannot stack toggle listeners; bump 0.5.2 (scadaproj#48) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Theme/CLAUDE.md | 7 ++ ZB.MOM.WW.Theme/Directory.Build.props | 2 +- .../ZB.MOM.WW.Theme/wwwroot/js/nav-state.js | 21 ++++- .../NavStateScriptTests.cs | 42 ++++++++++ .../ZB.MOM.WW.Theme.Tests/nav-state.test.js | 83 +++++++++++++++++++ code-reviews/README.md | 11 ++- code-reviews/Theme/findings.md | 6 +- 7 files changed, 158 insertions(+), 14 deletions(-) create mode 100644 ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavStateScriptTests.cs create mode 100644 ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/nav-state.test.js diff --git a/ZB.MOM.WW.Theme/CLAUDE.md b/ZB.MOM.WW.Theme/CLAUDE.md index d9e515c..3779774 100644 --- a/ZB.MOM.WW.Theme/CLAUDE.md +++ b/ZB.MOM.WW.Theme/CLAUDE.md @@ -4,6 +4,13 @@ Shared UI kit (Razor Class Library — Technical-Light tokens, IBM Plex, side-ra **Backlog / open work:** lives in Gitea issues on `dohertj2/scadaproj` (`tea issues list --repo dohertj2/scadaproj`) — currently #9 (roll `0.5.0` to the other consumers), #10 (`layout.css` `calc(100vh - 3.3rem)` review). The history below is a record, not a status tracker; file new follow-ups as issues. +## 0.5.2 (2026-11-09, UNPUBLISHED) + +Version bumped in `Directory.Build.props`, not packed or pushed. Fixes Theme-008 (scadaproj#48): `nav-state.js` tracks +wired sections in a JS `WeakSet` instead of the `data-zbnav-initialized` DOM attribute, which Blazor's enhanced-navigation +DOM sync strips (toggle listeners stacked and the transient active-link reveal was persisted). Regression test +`NavStateScriptTests` runs `nav-state.test.js` under Node (skips if Node is absent). + ## 0.5.1 (2026-11-09, UNPUBLISHED) Version bumped in `Directory.Build.props`, not packed or pushed. Fixes Theme-007 (scadaproj#44): diff --git a/ZB.MOM.WW.Theme/Directory.Build.props b/ZB.MOM.WW.Theme/Directory.Build.props index f17cecb..e13d9e1 100644 --- a/ZB.MOM.WW.Theme/Directory.Build.props +++ b/ZB.MOM.WW.Theme/Directory.Build.props @@ -4,7 +4,7 @@ enable enable latest - 0.5.1 + 0.5.2 true diff --git a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js index c318f5d..3e350d2 100644 --- a/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js +++ b/ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js @@ -14,8 +14,21 @@ if (summary) summary.setAttribute("aria-expanded", el.open ? "true" : "false"); } + // JS-side record of wired elements. The INIT_ATTR DOM attribute is NOT a reliable guard: Blazor's + // enhanced-navigation DOM sync keeps the
element but strips attributes the server did + // not render, so the marker disappears while our toggle listener stays attached - and a second + // wire() would stack another listener (Theme-008). A WeakSet (or expando fallback) is untouched + // by DOM sync. + var wired = typeof WeakSet === "function" ? new WeakSet() : null; + function isWired(el) { return wired ? wired.has(el) : el.__zbnavWired === true; } + function markWired(el) { + if (wired) wired.add(el); + else el.__zbnavWired = true; + } + function wire(el) { - el.setAttribute(INIT_ATTR, ""); + markWired(el); + el.setAttribute(INIT_ATTR, ""); // informational only; never read as a guard var key = PREFIX + el.getAttribute("data-nav-key"); var saved = null; try { saved = window.localStorage.getItem(key); } catch (e) { saved = null; } @@ -36,7 +49,7 @@ function apply() { document.querySelectorAll("details.rail-section[data-nav-key]").forEach(function (el) { - if (!el.hasAttribute(INIT_ATTR)) wire(el); // wire once — avoid duplicate listeners + if (!isWired(el)) wire(el); // wire once — avoid duplicate listeners syncAria(el); // re-sync aria on every pass }); @@ -60,7 +73,7 @@ // Re-run after Blazor static-SSR enhanced navigation (or any re-render that // replaces the rail nodes) so freshly inserted sections are wired, restored, - // and active-revealed (issue #3). The per-element INIT_ATTR guard keeps this + // and active-revealed (issue #3). The per-element wired guard keeps this // idempotent for nodes that survived the navigation. if (window.Blazor && typeof window.Blazor.addEventListener === "function") { window.Blazor.addEventListener("enhancedload", apply); @@ -72,7 +85,7 @@ // runtime adopts the page, and `enhancedload` does NOT fire — so without this // the live sections are never wired (no persistence, no aria sync, no // active-reveal). A MutationObserver is the render-mode-agnostic backstop; - // the per-element INIT_ATTR guard keeps re-applies idempotent, and the + // the per-element wired guard keeps re-applies idempotent, and the // childList-only filter (plus the active-reveal's `if (!sec.open)` guard) // avoids any observe→mutate→observe loop (issue #6). if (typeof MutationObserver === "function") { diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavStateScriptTests.cs b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavStateScriptTests.cs new file mode 100644 index 0000000..7b14b49 --- /dev/null +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/NavStateScriptTests.cs @@ -0,0 +1,42 @@ +using System.Diagnostics; +using System.Runtime.CompilerServices; + +namespace ZB.MOM.WW.Theme.Tests; + +/// +/// Theme-008: nav-state.js must not stack toggle listeners when Blazor's enhanced-navigation DOM +/// sync strips its marker attribute. Runs nav-state.test.js under Node (a DOM stub, no +/// jsdom); silently passes only when Node is not installed. +/// +public class NavStateScriptTests +{ + private static string Here([CallerFilePath] string path = "") => Path.GetDirectoryName(path)!; + + [Fact] + public void NavState_does_not_stack_listeners_or_persist_the_transient_reveal() + { + var script = Path.GetFullPath(Path.Combine(Here(), "..", "..", "src", "ZB.MOM.WW.Theme", "wwwroot", "js", "nav-state.js")); + var test = Path.Combine(Here(), "nav-state.test.js"); + + Process? node; + try + { + node = Process.Start(new ProcessStartInfo("node", $"\"{test}\" \"{script}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + }); + } + catch (System.ComponentModel.Win32Exception) + { + return; // Node not available on this machine. + } + + using (node!) + { + var output = node.StandardOutput.ReadToEnd() + node.StandardError.ReadToEnd(); + node.WaitForExit(); + Assert.True(node.ExitCode == 0, output); + } + } +} diff --git a/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/nav-state.test.js b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/nav-state.test.js new file mode 100644 index 0000000..2a3c526 --- /dev/null +++ b/ZB.MOM.WW.Theme/tests/ZB.MOM.WW.Theme.Tests/nav-state.test.js @@ -0,0 +1,83 @@ +// Behavioural test for nav-state.js (Theme-008), run by NavStateScriptTests under Node with a +// minimal DOM stub - no jsdom dependency. Exit code 0 = pass; failures print and exit 1. +"use strict"; +const fs = require("fs"); +const vm = require("vm"); + +function makeEl(attrs) { + const listeners = []; + const a = Object.assign({}, attrs); + let open = false; + const el = { + classList: { contains: () => true }, + getAttribute: (n) => (n in a ? a[n] : null), + setAttribute: (n, v) => { a[n] = String(v); }, + removeAttribute: (n) => { delete a[n]; }, + hasAttribute: (n) => n in a, + addEventListener: (t, f) => { if (t === "toggle") listeners.push(f); }, + querySelector: () => null, + closest: () => el, + get open() { return open; }, + set open(v) { if (v !== open) { open = v; listeners.slice().forEach((f) => f()); } }, + listenerCount: () => listeners.length, + attrs: a, + }; + return el; +} + +function run() { + const section = makeEl({ "data-nav-key": "admin" }); + const link = { closest: () => section, active: false }; + const store = { "zbnav:admin": "0" }; + const writes = []; + let enhancedLoad = null; + + const document = { + readyState: "complete", + documentElement: {}, + addEventListener() {}, + querySelectorAll(sel) { + if (sel === "details.rail-section[data-nav-key]") return [section]; + if (sel === "details.rail-section a.rail-link.active") return link.active ? [link] : []; + return []; + }, + }; + const window = { + document, + localStorage: { + getItem: (k) => (k in store ? store[k] : null), + setItem: (k, v) => { store[k] = v; writes.push([k, v]); }, + }, + Blazor: { addEventListener: (t, f) => { if (t === "enhancedload") enhancedLoad = f; } }, + }; + const ctx = { window, document, WeakSet, console }; + vm.createContext(ctx); + vm.runInContext(fs.readFileSync(process.argv[2], "utf8"), ctx); + + const fail = (m) => { console.error("FAIL: " + m); process.exit(1); }; + if (section.open) fail("saved '0' should leave the section collapsed"); + + // Three enhanced navigations; DOM sync strips attributes the server did not render. + for (let i = 0; i < 3; i++) { + Object.keys(section.attrs).forEach((k) => { if (k !== "data-nav-key") delete section.attrs[k]; }); + enhancedLoad(); + } + if (section.listenerCount() !== 1) fail("expected 1 toggle listener after re-apply, got " + section.listenerCount()); + + // The user expands, then collapses: exactly one write each. + section.open = true; + section.open = false; + if (writes.length !== 2) fail("expected 2 writes for expand+collapse, got " + writes.length); + store["zbnav:admin"] = "0"; + writes.length = 0; + + // Active-link reveal is transient and must not overwrite the saved preference. + link.active = true; + Object.keys(section.attrs).forEach((k) => { if (k !== "data-nav-key") delete section.attrs[k]; }); + enhancedLoad(); + if (!section.open) fail("active link should reveal its section"); + if (writes.length !== 0) fail("transient reveal persisted " + JSON.stringify(writes)); + if (store["zbnav:admin"] !== "0") fail("saved preference was overwritten"); + console.log("ok"); +} +run(); diff --git a/code-reviews/README.md b/code-reviews/README.md index bd8451b..4492e8b 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 34 pending findings across all libraries. +10 of 10 libraries reviewed. 33 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -51,8 +51,8 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Critical | 0 | | High | 0 | | Medium | 11 | -| Low | 23 | -| **Total** | **34** | +| Low | 22 | +| **Total** | **33** | ## Library Status @@ -67,7 +67,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | | [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/6/13 | 19 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 11 | -| [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 8 | +| [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | ## Pending Findings @@ -100,7 +100,7 @@ _None open._ | Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | | Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | -### Low (23) +### Low (22) | ID | Library | Title | |----|---------|-------| @@ -126,4 +126,3 @@ _None open._ | Telemetry-009 | [Telemetry](Telemetry/findings.md) | Enrichers added via `ZbSerilogOptions.ConfigureLogger` run after `RedactionEnricher`, so their properties are never redacted | | Telemetry-010 | [Telemetry](Telemetry/findings.md) | Redaction projection keys logged dictionaries by `ToString()`, silently dropping entries whose keys stringify identically | | Telemetry-011 | [Telemetry](Telemetry/findings.md) | OTLP fail-fast validation accepts scheme-less `host:port` and is skipped by the pre-built-options `AddZbTelemetry` overload | -| Theme-008 | [Theme](Theme/findings.md) | `nav-state.js` "wired" guard is a DOM attribute that Blazor enhanced-navigation DOM sync strips, so toggle listeners accumulate and the transient active-link reveal is persisted | diff --git a/code-reviews/Theme/findings.md b/code-reviews/Theme/findings.md index 9df859b..4de78bf 100644 --- a/code-reviews/Theme/findings.md +++ b/code-reviews/Theme/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | | Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | | Commit reviewed | `91fb2b4` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -345,7 +345,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#44)` (Theme `0.5.1`, unpubl | Severity | Low | | Priority | P3 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js:7`, `:17-18`, `:39`, `:65-66` | **Description** @@ -390,4 +390,4 @@ attribute. Add a test that strips attributes, re-runs `apply()`, and asserts one **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` (Theme `0.5.2`, unpublished) — `nav-state.js` now records wired sections in a JS `WeakSet` (expando fallback) instead of reading the `data-zbnav-initialized` attribute that Blazor's DOM sync strips; the attribute is still written but never used as a guard. Regression test `NavStateScriptTests` runs `nav-state.test.js` under Node with a minimal DOM stub: after 3 simulated enhanced navigations (attribute stripped) there must be 1 toggle listener, expand+collapse must be 2 writes, and the transient active-link reveal must write nothing and leave `zbnav:admin=0` (failed with 4 listeners without the fix). The test passes vacuously on machines without Node (noted in its summary).