fix/bill-parity
needs attentionviewing older commit70da331 · incrementalpre-PRreviewed 2026-08-05 20:04 UTC2H · 3M · 6L- Purpose
- Achieve billing parity with the legacy Python/Smarter bill pipeline on two dimensions: (1) max_demand_15min TOU classification and (2) Energía Real brand embedding in PDF output.
- Goal
- Fix max_demand_15min to use whole-window-in-band (not start-ts-only) classification to match legacy daily-aggregation.py; embed the Energía Real brand as the default bill brand so the renderer is self-contained.
- Sub-goals
- fix(cross-domain): classify max_demand_15min by whole-window-in-band — drops boundary-straddling windows matching legacy period=None behaviour (+$152 Capacidad over-billing repro fixed)
- feat(bill-pdf): embed Energía Real (THOR/Smarter) brand as base64 defaults via code-generation script
- fix(bill-pdf): legacy header + chart-format parity (prior commit)
- fix(bill-pdf): Y-axis on PDBT monthly chart (prior commit)
- What
- Rolling 15-min demand peak classification now checks both start and end of the window against TOU band boundaries, dropping straddlers. Three PNG brand assets embedded as base64 data URIs via a new build script. bill-document.tsx now spreads DEFAULT_BRAND as the base brand with vm.brand as override.
- Why
- Legacy daily-aggregation.py assigns period=None to any 15-min window that doesn't fit entirely within a single band — AREAS COMUNES' 17:50 window was over-billing Capacidad by +$152/month by leaking into punta. Brand embedding makes the SFN payload lean (no per-bill logo bytes).
- Areas
- domains/cross-domain+71−30packages/bill-pdf+89−5
- Blast
- 10 files, +160/−25 lines across cross-domain (metrics) and bill-pdf (rendering). No schema changes, no migrations, no API surface changes.
Findings · 11
security1
Unvalidated vm.brand spread into PDF image src
packages/bill-pdf/src/bill-document.tsx:182
{ ...DEFAULT_BRAND, ...vm.brand } allows any caller string to reach <Image src>. Risk is bounded if vm.brand is DB-gated upstream; worth asserting data:image/png;base64, prefix at the brand-override boundary.
conventions1
FIFTEEN_MIN_MS declared inside function body
domains/cross-domain/src/site-energy-metrics.shells.ts
A module-level concept (15 minutes in milliseconds) declared inside the loop body; minor — prefer hoisting to module scope.
tests6
Missing test: base/intermedio boundary (06:00) straddler
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
The new whole-window-in-band logic applies to ALL band boundaries (06:00, 18:00, 22:00). Only the 22:00 punta→intermedio boundary is tested. A window straddling 06:00 (e.g. starting 05:50, ending 06:04:59) should also be dropped — the off-by-one at the lower boundary is independent code.
Missing test: window starting exactly on a band boundary counts (inclusive lower edge)
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
A window whose start ts is exactly 18:00 and end ts is 18:14:59.999 should be fully in punta and counted. No test covers this — a fence-post regression (start > boundary instead of start >= boundary in decideTouPeriod) would go undetected.
Missing test: weekend/base-only schedule (punta=0, no straddler possible)
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
SIN weekends have only base band. The code must produce 0 (not NaN/undefined) for punta and intermedio max_demand_15min. No test exercises a Saturday/Sunday input series with the new classification path.
Missing test: coarser slotSeconds (15-min/hourly) with boundary classification
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
FIFTEEN_MIN_MS hardcodes 15*60*1000 to compute endBand. For slotSeconds=900, rolling15minMeansW returns single-slot windows — boundary behaviour differs. No test for slotSeconds != 300.
Intermedio 140 kW assertion doesn't enumerate all intermedio windows
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
The test asserts 140 kW is the intermedio max without confirming no 06:00–18:00 window exceeds 60 kW. A comment would make this self-documenting.
No explicit 220 kW sentinel tying assertion to old behaviour
domains/cross-domain/src/__tests__/site-energy-metrics-breakdown.test.ts
Comment notes 'old code would have said 220' but toBeLessThan(200) or a comment anchoring the regression would make the intent explicit for future refactors.
improvement3
FIFTEEN_MIN_MS detached from rolling15minMeansW's actual window duration
domains/cross-domain/src/site-energy-metrics.shells.ts
The constant 15*60*1000 duplicates the semantic contract of rolling15minMeansW. If slotSeconds changes (e.g. 15-min CSV sources), the endBand classification silently uses the wrong duration. Deriving the offset from slotSeconds would make the invariant structural.
Committed base64 PNGs inflate git history permanently
packages/bill-pdf/src/brand-assets.generated.ts
~100KB of base64 in a committed generated file adds non-delta-compressible blobs on every regeneration. Consider generating as a prebuild step and gitignoring the generated file, keeping only source PNGs in version control.
new Date() allocation inside hot loop is avoidable
domains/cross-domain/src/site-energy-metrics.shells.ts
periodOf memoizes on ts.getTime(). The new Date allocation only feeds getTime() — passing the numeric timestamp directly (if periodOf accepted number) would avoid the allocation. Trivial at ~288 windows/day.