refactor: consolidate stat-card markup in static analytics template (#4923)

[frano-m]

Closes #4923

Note: #4930 (fran/4922-access-request-service-cards) has merged, so this branch now sits directly on main. The two commits belonging to this PR are 98f35b3c (the consolidation) and 960ccc00 (sign-before-rounding + shared changeClass).

What changed

The stat-card markup in the shared static analytics template was hand-written in three places and had diverged (tooltip/change-row/grid-class combinations). This PR consolidates all three onto the existing statCard() helper, extended with an options object:

• statCard(value, label, { change, gridClass, tooltip }) — omitting change omits the change row; null renders "N/A vs prior month"; multi-line labels (\n) render as line breaks; gridClass is omitted when the container sizes cards itself (access-requests grid).
• Top stats grid (renderStats): output equivalent — the rendered change text is identical across all change states (positive, negative, null, zero). Not byte-identical: changeHtml is now built in its own template literal and interpolated, so the <div class="stat-change"> block carries different leading/trailing whitespace and indentation. HTML-insignificant, no visual effect.
• Key metrics (renderEventCounts): inline copy deleted; now also reuses pctChange() instead of hand-rolling the same formula.
• Access-requests stats: inline copy deleted.
• pctChange() hardened with a Number.isFinite guard (a non-finite result now renders "N/A vs prior month" instead of "NaN%" — pre-existing, practically unreachable edge).
• Change sign is now derived before rounding, so a small negative like -0.04% no longer renders as "+-0.0%" in green; changeClass() is shared across all call sites.

Two deliberate deltas, flagged for anyone pixel- or diff-comparing:

• Event-count and access-request labels now sit in the same stat-label-wrap wrapper as the top stats, which adds a few pixels of label height on those cards — that's the unification working as intended.
• A key-metrics card with no usable baseline (prior is 0 or missing — e.g. a metric introduced this month) previously rendered +0% vs prior month, because the old code interpolated the number 0 raw. It now routes through Number(change).toFixed(1) and renders +0.0%. Purely cosmetic, and consistent with the other cards now that they all round to one decimal.

How verified

• Node equivalence tests: unified statCard() vs the legacy markup for all three shapes and every change-value path; pctChange() unit cases including the NaN guard.
• Playwright rendering tests: top stats (4 cards, tooltips work on hover), key metrics (6 cards, multi-line labels, change rows, fui-grid-item-6), access-request cards (no change rows), plus the full fix: generalize access-request service classification and stat cards in static analytics template #4922 regression suite (multi-service, single-service, malformed payloads) — zero JS errors.
• /code-review at high effort: no correctness bugs; the two cleanup findings it raised are applied in this PR, the label-wrap geometry note is the deliberate delta above.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This pull request refactors the shared static analytics template to consolidate duplicated “stat card” HTML into a single statCard() helper with an options object, while also tightening up change-percentage calculation and the access-requests stats/table rendering logic.

Changes:

• Expanded statCard(value, label, { change, gridClass, tooltip }) to support optional change rows, optional grid classes, tooltips, and multi-line labels.
• Updated the top stats grid (renderStats) and key metrics (renderEventCounts) to reuse statCard() and pctChange().
• Reworked access-requests rendering to accept pre-fetched JSON, compute per-service counts generically, and render per-service cards dynamically.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[frano-m]

Follow-up review pass (9e3df872) — fixed a real defect the consolidation exposed, plus the cleanups it invited.

The defect: pctChange() returned a toFixed(1) string, so statCard tested the sign after rounding. A tiny decline (e.g. current 9999 vs prior 10000 → -0.01% → "-0.0") coerces to -0, which passes >= 0, so the card rendered +-0.0% vs prior month in green. Reproduced before the fix; pctChange() now returns a number and statCard rounds at render time, so the sign comes from the true value.

Also in this commit:

• aria-label escaping — escapeHtml doesn't escape double quotes, and its output landed inside a double-quoted attribute; a label with a quote could inject attributes. Now quote-encoded (and newlines collapsed to spaces for multi-line labels).
• Shared changeClass() — the positive/negative ternary existed in four places (statCard + the three table renderers). Extracted and used at all four; valid for both the fractions tables carry and the percentages cards carry.
• Escape the label once instead of N+1 times per card, dropping the redundant String() coercion and the second escape call site.
• Flattened the paired nested ternaries (the sign test was evaluated three times) and single-sourced the event-label newline split, which happened up to four times per card.

One item deliberately not applied, flagged for a call: renderEventCounts still passes pctChange(...) ?? 0, so an event with no prior-month baseline shows +0.0% in green rather than the N/A vs prior month that statCard now supports (and that the traffic cards show for the same condition). Four review angles flagged it. It's base-parity behaviour, but changing it changes what published reports display — happy to switch it to N/A if that's the preferred semantics.

Verified: display parity against real AnVIL fixture data (−19.9% / −23.7% / −31.7% / +7.1% identical before and after), all three card families, table change cells still coloured, tooltips working, malformed-payload path intact, zero JS errors.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[NoopDog]

Approving — the consolidation is behaviour-preserving and both intended fixes work (-0.04% no longer renders as "+-0.0%" in green, and a non-finite ratio renders N/A instead of NaN%).

Verified: !(prior > 0) exactly negates the old guard; changeClass's == null matches the old != null checks at all three table call sites; Number(change).toFixed(1) reproduces the old pre-rounded string; the pairLabelLines hoist reads the same card.label properties as before; escapeHtml leaves newlines and quotes untouched, and the " substitution correctly happens after & escaping. All six statCard/pctChange/changeClass call sites are updated, and generator.py does a plain shutil.copy2 (no Jinja), so the new destructuring braces are safe.

Two notes on the PR description rather than the code:

• "byte-identical" top stats grid — the change text is identical, but changeHtml is now built in its own template literal and interpolated, so the <div class="stat-change"> block carries different leading/trailing whitespace and indentation. HTML-insignificant, no visual effect — just not byte-identical.
• Stale commit SHA — the description says only 63859e11 belongs to this PR, but the branch carries two #4923 commits on top of #4930: 98f35b3c (refactor) and 960ccc00 (sign-before-rounding + shared changeClass). Worth correcting so reviewers of the stack look at the right range.