scadaproj/code-reviews/Theme/findings.md

Code Review — Theme

Field	Value
Library	ZB.MOM.WW.Theme/
Packages	ZB.MOM.WW.Theme (Razor Class Library)
Component spec	components/ui-theme/spec/SPEC.md
Shared contract	components/ui-theme/shared-contract/ZB.MOM.WW.Theme.md
Status	Reviewed
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	0

Summary

2026-11-09 re-review (91fb2b4, scadaproj#15). Re-reviewed at 91fb2b4 (2026-11-09) against the 2026-06-01 baseline 5f75cd4; the kit is now 0.5.0. New since the baseline:

• the CSS-only <details> app shell, with the ::details-content fix and the mobile shell fix;
• nav-state.js (localStorage persistence of NavRailSection, active-link reveal, and a MutationObserver backstop for interactive render modes);
• ThemeScripts;
• the TechButton Size/Class extensions;
• the stateless TreeTable<TRow>/ITreeRow;
• the upstreamed .btn sizing block and the 0.5.0 presentation classes.

The C# components hold no state, implement no IDisposable and hold no JS interop references. There is no MarkupString anywhere, and every caller-supplied string is attribute- or text-encoded by Razor. LoginCard still delegates ReturnUrl validation and antiforgery to the host, with explicit docs. TreeTable documents its Bootstrap Icons dependency.

The MutationObserver is never disconnected. That is fine: it is page-lifetime, created once per document, and the observe→mutate loop is guarded. The <details> shell relies on author display rules for browsers without ::details-content. That could not be verified without those browsers, so it is not raised.

All 60 bUnit/static-asset tests pass. There are two new findings:

• Theme-007 (Medium): TechButton silently discards a caller-supplied disabled attribute. The explicit disabled="@Busy" is written after the splat, so a button the caller meant to disable renders enabled. Verified with a bUnit probe.
• Theme-008 (Low): nav-state.js marks a section as wired with a DOM attribute. Blazor's enhanced-navigation DOM sync strips that attribute, so toggle listeners pile up on each navigation, and the extra listeners save the transient active-link reveal as the user's preference, which is the issue #2 regression. Verified by reading the code and simulating it in jsdom, with a control.

2026-06-01 baseline (5f75cd4). ZB.MOM.WW.Theme is a small, clean, well-scoped Blazor Razor Class Library: ten flat-namespace SSR components, two stylesheets (theme.css 379 lines, layout.css 191 lines), three vendored IBM Plex .woff2 fonts, and two public enums. The component C# is correct and minimal — stateless render logic, no concurrency surface, no secret handling, sensible [Parameter]/EditorRequired choices, and TreatWarningsAsErrors. The 32 bUnit tests (20 [Fact] + 12 [Theory] cases) exercise the public render contract of every component plus a static-asset check on the corrected font path. The standout security-sensitive area — LoginCard (open-redirect + antiforgery) — is handled correctly by delegating both concerns to the consumer with explicit XML docs and in-markup security notes, matching the spec. No findings are Critical or High. The findings are concentrated in two themes: (1) one functional gap — the mobile hamburger relies on Bootstrap's collapse JS, an undocumented runtime dependency that contradicts the "no JavaScript" framing; and (2) CSS / token hygiene — a token-fidelity drift on .chip-idle, hardcoded hex values that violate the spec's "no hardcoded hex" rule, an undefined .rail-ico class, orphan/dead nav CSS, and a near-total absence of XML docs on the public parameter surface.

Checklist coverage

2026-11-09 re-review at 91fb2b4:

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	TechButton drops caller disabled → Theme-007. Duplicate toggle listeners after enhanced navigation persist the transient reveal → Theme-008. TreeTable (keyed rows, leaf ARIA omission, invariant-culture depth style) and the NavRailSection slug are correct.
2	Public API surface & compatibility	☑	Additive in 0.2.0–0.5.0: ButtonSize, TechButton.Size/Class, TreeTable/ITreeRow, NavRailSection.Key, LoginCard.Heading, ThemeScripts. No removals. The CaptureUnmatchedValues contract is not fully honoured for disabled (Theme-007).
3	Concurrency & thread safety	☑	Stateless components; the JS is single-threaded with idempotent apply(). No issues found.
4	Error handling & resilience	☑	localStorage access is wrapped in try/catch, and the script degrades when window.Blazor or MutationObserver is absent. No issues found.
5	Security & secret handling	☑	No MarkupString and no raw HTML. Accent goes into a style attribute but is developer-supplied. LoginCard ReturnUrl is echoed into an encoded hidden field, and the open-redirect check stays documented as the host's job. localStorage holds only open/closed flags. No issues found.
6	Performance & resource management	☑	The MutationObserver (whole-document childList/subtree) is page-lifetime and cheap per mutation. Toggle listeners accumulate one per enhanced navigation (Theme-008). No IJSObjectReference or IDisposable to leak.
7	Spec & shared-contract adherence	☑	No-JS shell holds: native <details>, no Bootstrap JS. Tokens and chips are unchanged apart from documented additions. No new drift found.
8	Packaging, dependencies & project layout	☑	0.5.0 RCL, FrameworkReference only. nav-state.js is shipped and asserted by StaticAssetsTests. AngleSharp is pinned to 1.5.0 in the test project only. No issues found.
9	Testing coverage	☑	60 tests pass. Gaps: no test passes disabled through TechButton's splat (Theme-007), and nothing exercises nav-state.js under DOM sync (Theme-008).
10	Documentation & XML docs	☑	Theme-006 fix holds (XML docs on every component, parameter and enum). README documents TreeTable's Bootstrap Icons and interactivity requirements. No issues found.

2026-06-01 baseline checklist (5f75cd4)

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Component render logic correct; open="@Expanded" boolean-attribute binding behaves (tests confirm); chip/variant switch mappings complete. No issues found.
2	Public API surface & compatibility	☑	Flat ZB.MOM.WW.Theme namespace; minimal, intentional; EditorRequired applied; nullable annotations correct; no internal types leak. Surface matches contract. No issues found.
3	Concurrency & thread safety	☑	Stateless components, no shared mutable state, no async/statics/singletons. No issues found (UI kit).
4	Error handling & resilience	☑	No throwing paths; required params default to string.Empty; null RenderFragment slots are guarded (is not null / IsNullOrEmpty). No issues found.
5	Security & secret handling	☑	No secrets/PII. LoginCard open-redirect + CSRF correctly delegated to consumer with docs. Accent interpolates into a style attr but is developer-supplied (not untrusted). No issues found.
6	Performance & resource management	☑	No IDisposable, no allocations of note, fonts vendored once. No issues found.
7	Spec & shared-contract adherence	☑	Component/parameter API matches SPEC §4 and the shared contract. Drifts found: .chip-idle foreground (Theme-002) and "no hardcoded hex" rule (Theme-003).
8	Packaging, dependencies & project layout	☑	net10.0 RCL, single package id ZB.MOM.WW.Theme, FrameworkReference only, version 0.1.0 (inherited from Directory.Build.props), static assets under _content/. No issues found.
9	Testing coverage	☑	32 tests cover every component's render contract + corrected font path. CSS-class wiring (e.g. .rail-ico, orphan nav CSS) is not asserted, which is how Theme-004/005 slipped through.
10	Documentation & XML docs	☑	README/contract accurate. XML docs present only on LoginCard; absent on the other 9 components, all their parameters, and both enums (Theme-006).

Findings

Theme-001 — Mobile hamburger toggle silently depends on Bootstrap collapse JS

Severity	Medium
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor:5

Description

ThemeShell renders the narrow-viewport navigation toggle as a Bootstrap collapse trigger: <button ... data-bs-toggle="collapse" data-bs-target="#theme-rail" aria-expanded="false">, with the rail wrapped in <div class="collapse d-lg-block" id="theme-rail">. The expand/collapse behaviour and the aria-expanded state transition are driven entirely by Bootstrap's collapse JavaScript bundle at runtime. The shared contract states "No global JavaScript … NavRailSection is CSS-only" and the spec emphasizes static-SSR/no-JS operation, while §5 only requires each app to keep its own Bootstrap <link> (CSS) — Bootstrap JS is never mentioned. A consumer that includes only Bootstrap CSS (a reasonable reading of the docs) gets a hamburger button that does nothing below the lg breakpoint: the rail stays display:none (.collapse) and is unreachable on mobile/narrow windows. There is no compile-time signal, and the hardcoded aria-expanded="false" never updates without the JS, so assistive tech is also misinformed. This affects every consumer's small-viewport navigation.

Recommendation

Either (a) document the Bootstrap-JS runtime requirement explicitly in the README/spec adoption steps ("the side-rail mobile toggle requires Bootstrap's collapse JS bundle"), or (b) make the toggle JS-independent to match the "no JavaScript" promise — e.g. wrap the rail in a <details>/checkbox CSS-only disclosure (the same pattern NavRailSection already uses), so collapse works in pure static SSR. If keeping the Bootstrap-collapse approach, document that aria-expanded requires the JS to stay accurate.

Resolution

Resolved in 544a6dd, 2026-06-01 — removed the Bootstrap-collapse JS dependency: the shell is now a native CSS-only <details>/<summary> disclosure (no data-bs-*), force-shown on lg+; bUnit test asserts no Bootstrap collapse hooks.

Theme-002 — .chip-idle foreground diverges from the documented token pairing

Severity	Low
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css:177

Description

DESIGN-TOKENS.md ("Status tokens — tinted backgrounds") states the chip classes pair each tinted background with its matching foreground token — "--ok-bg background with --ok foreground … Used by .chip-ok, .chip-warn, .chip-bad, .chip-idle". The --idle token (#868e96) is correctly used for .s-idle and the .conn-pill .dot, but .chip-idle uses color: var(--ink-soft) (#5a6066) instead of color: var(--idle):

.chip-idle { color: var(--ink-soft); background: var(--idle-bg); border-color: var(--rule-strong); }

So the one idle chip variant does not follow the pairing rule the design-tokens doc spells out. It is not a contrast or readability regression (--ink-soft is darker than --idle), but code and the normalized token reference disagree, which is exactly the spec-drift category §7 asks to flag. Note the .chip-warn foreground is likewise a hardcoded #b56a00 rather than var(--warn) — see Theme-003.

Recommendation

Either change .chip-idle to color: var(--idle) to honor the documented pairing, or, if the darker --ink-soft ink is the deliberate choice, amend DESIGN-TOKENS.md to record that .chip-idle uses --ink-soft foreground (as it already does for the Info variant footnote).

Resolution

Resolved in 544a6dd, 2026-06-01 — changed .chip-idle foreground from var(--ink-soft) to var(--idle), honoring the DESIGN-TOKENS pairing rule; static-asset test asserts the pairing.

Theme-003 — Hardcoded hex values in CSS contradict the "no hardcoded hex" rule

Severity	Low
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/theme.css:174 (and theme.css:96-98,145-150,175-176,251-254,281,290-291,335,348,368-369,377; layout.css:123,128,184)

Description

SPEC.md §1 and DESIGN-TOKENS.md both assert the rule: "Components carry no hardcoded hex values — everything references these tokens" / "no hardcoded hex values appear in component markup or CSS". In practice both stylesheets contain numerous literal hex colors outside the :root token block — chip/agg border tints (#c6e6cd, #eec3c3, #efd6a6, #bfe3c6, #f0d9ab, #f0c0c0), chip/notice foregrounds (#b56a00, #8a5a00), zebra/hover fills (#fbfbf9, #f3f6fd, #eef2fc), and the info/dir tints (#e7ecfb, #cdd9f7). These are derived shades (lighter borders, hover washes) with no corresponding token, so they are not re-themable via the token block and any per-app accent override will not reach them. The rule as written is therefore aspirational, and the DESIGN-TOKENS.md Info footnote itself already references a raw #e7ecfb, so the doc partially contradicts its own rule.

Recommendation

Reconcile code and doc: either promote the recurring derived shades to named tokens (e.g. --ok-border, --hover-bg, --info-bg/--info-border) and reference them, or soften the spec wording to "no hardcoded hex in component markup; CSS derives border/hover shades from tokens where practical" so the rule matches reality. This keeps the "token changes are breaking" SemVer guarantee honest.

Resolution

Resolved in 544a6dd, 2026-06-01 — promoted every derived border/ink/wash shade to named :root tokens (--ok-border, --warn-ink, --info-bg, --hover-bg, --active-bg, --zebra-bg, …) and referenced them; no hex literal remains outside :root in either stylesheet, asserted by a new test.

Theme-004 — NavRailItem emits a .rail-ico span that no stylesheet defines

Severity	Low
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailItem.razor:4

Description

When the optional Icon slot is supplied, NavRailItem wraps it: <span class="rail-ico">@Icon</span>. Neither theme.css nor layout.css defines a .rail-ico rule (confirmed by grep). The icon therefore renders as an unstyled inline span — no sizing, alignment, or gap relative to the label text — so an icon + text rail link will not line up the way the other rail styling implies. It is not a crash, but the component ships a class hook with no backing style, which is a latent presentation bug for any consumer that uses the Icon parameter (and none of the 32 tests cover the icon path).

Recommendation

Add a .rail-ico rule to layout.css (e.g. display:inline-flex; width:1rem; margin-right:0.4rem; color:var(--ink-faint);) alongside the existing .rail-link rules, and add a bUnit test asserting the .rail-ico span is emitted when Icon is supplied.

Resolution

Resolved in 544a6dd, 2026-06-01 — added a .rail-ico rule to layout.css (inline-flex, sized, gapped, --ink-faint); bUnit tests assert the span is emitted when Icon is supplied and omitted otherwise.

Theme-005 — Orphan and unstyled nav CSS classes in layout.css

Severity	Low
Category	Documentation & XML docs
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/css/layout.css:75 (.rail-eyebrow), :103 (.rail-eyebrow-chevron); ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/NavRailSection.razor:5 (.rail-eyebrow-label)

Description

NavRailSection renders <summary class="rail-eyebrow-toggle"><span class="rail-eyebrow-label">@Title</span></summary> and layout.css provides the actual collapse chevron via .rail-section > summary::before (lines 178-181). That leaves several mismatched class hooks: .rail-eyebrow (line 75, non-toggle) and .rail-eyebrow-chevron (lines 103-108) are defined in CSS but emitted by no component (dead CSS), while .rail-eyebrow-label is emitted by the component but has no CSS rule (it merely inherits from .rail-eyebrow-toggle). The result is two parallel chevron mechanisms (the styled-but-unused .rail-eyebrow-chevron and the actually-used ::before) and stylesheet cruft that misleads future maintainers about how the section header is themed.

Recommendation

Remove the dead .rail-eyebrow and .rail-eyebrow-chevron rules (or wire .rail-eyebrow-chevron into NavRailSection and drop the ::before approach — pick one), and either add a .rail-eyebrow-label rule or drop the now-redundant wrapper span. Keeping one chevron mechanism removes the ambiguity.

Resolution

Resolved in 544a6dd, 2026-06-01 — removed the dead .rail-eyebrow and .rail-eyebrow-chevron rules and dropped the redundant .rail-eyebrow-label wrapper span in NavRailSection, leaving one chevron mechanism (summary::before).

Theme-006 — Public component/parameter surface lacks XML documentation

Severity	Low
Category	Documentation & XML docs
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/ThemeShell.razor:23 (and BrandBar.razor, NavRailItem.razor, NavRailSection.razor, StatusPill.razor, TechButton.razor, TechCard.razor, TechField.razor, ThemeHead.razor, ButtonVariant.cs, StatusState.cs)

Description

This is a packaged library (IsPackable=true, 0.1.0) consumed by three separate apps, and REVIEW-PROCESS.md §2.10 calls for "XML doc accuracy and presence on the public API (these are libraries — public docs matter)." Only LoginCard carries XML docs (on ReturnUrl and ChildContent). The other nine components, all of their [Parameter] properties, and both public enums (ButtonVariant, StatusState) have no /// documentation, so consumers get no IntelliSense/tooltip help on the parameter contract (e.g. that ThemeShell.Accent overrides the --accent token, that NavRailItem.Match defaults to Prefix, or what each StatusState/ButtonVariant member maps to). The README and shared contract document all of this, but that prose is not surfaced at the call site.

Recommendation

Add brief /// summaries to the public components, their parameters, and the two enum types — at minimum the non-obvious ones (ThemeShell.Accent, NavRailItem.Match, TechButton.Busy, TechCard.Title vs Header precedence, and each enum member). Consider enabling GenerateDocumentationFile so missing-doc warnings surface under the existing TreatWarningsAsErrors.

Resolution

Resolved in 544a6dd, 2026-06-01 — added /// summaries to all nine remaining components, every public [Parameter], and both enums (ButtonVariant, StatusState); enabled GenerateDocumentationFile (CS1591 left off the error set for Razor-generated members) so docs build under TreatWarningsAsErrors with 0 warnings.

Theme-007 — TechButton silently discards a caller-supplied disabled attribute

Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor:3

Description

The button is rendered as <button @attributes="AdditionalAttributes" type="@Type" class="@ButtonClass" disabled="@Busy">. In Blazor, an attribute written after an @attributes splat overrides the same-named splatted attribute. disabled is not a declared parameter, so a caller's disabled="…" is captured into AdditionalAttributes and then overwritten by disabled="@Busy". When Busy is false, Blazor drops the attribute entirely. 0.5.0 added a merge for class (ButtonClass folds in the splatted class), but disabled was not merged. The XML doc on AdditionalAttributes says "arbitrary additional HTML attributes splatted onto the underlying <button>". The existing test Class_composition_keeps_busy_type_and_click_behavior covers only the opposite direction (Busy=true beating caller disabled=false).

Failure scenario

A page renders <TechButton Variant="ButtonVariant.Danger" disabled="@(!CanDelete)" @onclick="Delete">Delete</TechButton> for a user who cannot delete, or for a form that is not yet valid. The output is <button type="button" class="btn btn-danger">Delete</button>, with no disabled, so the button is clickable and fires Delete. A bUnit probe in the scratchpad confirmed this: Render<TechButton>(p => p.AddUnmatched("disabled", true)) produced <button type="button" class="btn btn-primary">Delete</button>. No current family call site passes disabled (20 .razor files use TechButton), so this is latent. It is still a natural use of the documented splat, and any server-side authorization check would be the only backstop.

Recommendation

Compute the effective state as Busy || callerDisabled, where callerDisabled is read from AdditionalAttributes["disabled"] with the same case-insensitive lookup used for class. Better still, add an explicit [Parameter] public bool Disabled. Then render disabled="@(Busy || Disabled)". Add a bUnit test for caller disabled=true with Busy=false.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#44) (Theme 0.5.1, unpublished) — TechButton renders disabled="@(Busy || CallerDisabled)", where CallerDisabled reads the splatted disabled case-insensitively with Blazor's own splat semantics (false/null omit it; true, "disabled", "" render it). Chose the lookup over a new [Parameter] bool Disabled because Blazor binds parameters case-insensitively: a bool parameter would turn an existing string disabled="disabled" splat into a runtime cast failure. Regression tests TechButtonExtensionsTests.Caller_disabled_is_honoured_when_not_busy (true, "disabled", ""), Caller_disabled_false_leaves_the_button_enabled and Caller_disabled_toggles_off_on_rerender; the first and third fail without the fix.

Theme-008 — 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

Severity	Low
Priority	P3
Category	Correctness & logic bugs
Status	Resolved
Location	ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js:7, :17-18, :39, :65-66

Description

wire() marks a section as initialised with the attribute data-zbnav-initialized. apply() uses hasAttribute(INIT_ATTR) as its only guard against adding a second toggle listener (commit 9e4dedc, "guard nav-state.js against duplicate toggle listeners"). After an enhanced navigation, Blazor's DOM sync (blazor.web.js 10.0.1, its attribute synchronisation step) keeps the existing <details> element but removes every attribute that is not in the newly server-rendered HTML. That includes data-zbnav-initialized, which the server never renders. Then enhancedload calls apply(), which finds no marker and calls wire() again, adding another toggle listener to the same element. After N navigations the element carries N+1 listeners.

The transient-reveal protection assumes a single listener. The first listener consumes data-zbnav-transient and returns. Every later listener sees no flag and writes localStorage[zbnav:<key>].

Failure scenario

1. A user collapses the "Admin" rail section; zbnav:admin is saved as "0".
2. They move around the app by enhanced (static-SSR) navigation.
3. They reach a page whose active link lives inside "Admin", for example from a link in the page body or a redirect.
4. apply() reveals the section and flags it transient.

The duplicate listeners then overwrite the saved preference with "1", so the section stays expanded on every later page. This is the issue #2 bug the transient flag exists to prevent.

A jsdom simulation reproduced it, running the shipped nav-state.js, stripping the attribute the way DOM sync does, and firing enhancedload 3 times:

• one collapse produced 4 setItem writes;
• the reveal produced 3 writes of "1", leaving saved=1;
• the control run, with the attribute not stripped, produced 1 collapse write and 0 reveal writes, leaving saved=0.

Recommendation

Track wiring in a JS-side structure that DOM sync cannot touch, for example a module-level WeakSet of wired elements or an expando property (el.__zbnavWired = true), instead of a DOM attribute. Add a test that strips attributes, re-runs apply(), and asserts one write per toggle.

Resolution

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).