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
This commit is contained in:
Joseph Doherty
@@ -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):
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
<Nullable>enable</Nullable>
|
||||
<ImplicitUsings>enable</ImplicitUsings>
|
||||
<LangVersion>latest</LangVersion>
|
||||
<Version>0.5.1</Version>
|
||||
<Version>0.5.2</Version>
|
||||
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
|
||||
</PropertyGroup>
|
||||
</Project>
|
||||
|
||||
@@ -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 <details> 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") {
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
using System.Diagnostics;
|
||||
using System.Runtime.CompilerServices;
|
||||
|
||||
namespace ZB.MOM.WW.Theme.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Theme-008: nav-state.js must not stack toggle listeners when Blazor's enhanced-navigation DOM
|
||||
/// sync strips its marker attribute. Runs <c>nav-state.test.js</c> under Node (a DOM stub, no
|
||||
/// jsdom); silently passes only when Node is not installed.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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();
|
||||
@@ -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 |
|
||||
|
||||
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user