feat/billing-improvements
needs attentionf406509 · incrementalpre-PRreviewed 2026-07-30 22:31 UTC5H · 8M · 8L · 5I- Purpose
- Consolidate the bill-PDF rendering pipeline so the interactive HTTP endpoint and the batch SFN worker render identical PDFs from identical logic — eliminating silent drift between the two paths.
- Goal
- Single canonical view-model mapper in @batu/cross-domain, DST-safe and TOU-aware chart generation, and A4 chart-width fix.
- Sub-goals
- SG-1: Hoist toBillPdfViewModel + buildBillPdfRichMeta from the platform handler into @batu/cross-domain (shared by batch worker)
- SG-2: Fix dayKeys() to anchor to local-midnight calendar boundaries (non-midnight-aligned windows no longer drop the last partial day)
- SG-3: Add GDMTH TOU daily chart (hasSchedules guard + dailyTou output)
- SG-4: Guard calendarMonthly series (1M sources produce garbage per-day bars)
- SG-5: Fix A4 chart width clipping (CHART_W 523→480)
- SG-6: Delete the replaced bill-charts.shells.ts (1h-resolution approach) and local bill-pdf.mapper.ts
- What
- New bill-pdf-view-model.ts in cross-domain consolidates all ComputeBillResult→BillPdfViewModel mapping. bill-charts-from-series.ts gets localMidnightUtc fixpoint, calendarMonthly guard, and empty-schedules fix. New TOU/DST tests added. Old bill-charts.shells.ts and bill-pdf.mapper.ts deleted.
- Why
- The platform handler and batch Lambda had separate, diverging toViewModel implementations; charts from the old 1h-resolution shell required an extra Tinybird read; the dayKeys bug silently dropped the last chart bar on non-midnight-aligned billing windows.
- Areas
- domains/cross-domain+291−143packages/bill-pdf+218−118services/billing+117−0apps/platform/src/api+23−126infra/cdk+191−13pnpm-lock.yaml+21−0
- Blast
- 27 files, +1102/−351 lines across billing pipeline, CDK infra, and PDF renderer
Findings · 28
correctness7
Due-date calculation semantics need verification
domains/cross-domain/src/bill-pdf-view-model.ts
The comment says 'inclusive_end + 13 days', but `inclusiveEnd = periodEnd - 1 day` then `due = inclusiveEnd + 13`. Net result: due = periodEnd + 12 days (from the exclusive boundary). If the callers' `periodEnd` is already the exclusive next-period-start this is correct (inclusive last day + 13 = exclusive end + 12). But if any caller passes an inclusive end, the due date is off by 1. Both callers (platform handler and batch worker) must be confirmed to pass an exclusive-end UTC value.
fmtBillDateDDMMYYYY uses UTC components — wrong for UTC-positive timezones
services/billing/src/handlers/generate-bill-pdf.handler.ts
The old handler used a timezone-aware `fmt(d, tz)` via Intl.DateTimeFormat. The new `fmtBillDateDDMMYYYY` uses `getUTCDate/getUTCMonth/getUTCFullYear`. This is correct today because all production sites are UTC-negative (CDMX UTC-6, Tijuana UTC-7/8), where `fromUtc` is local midnight as UTC (e.g. 06:00Z = midnight CDMX). For a UTC+ site, fromUtc local midnight is e.g. 18:30Z the previous calendar day — getUTCDate() would return the wrong date. The platform handler has the same assumption. Needs a guarding precondition or tz-aware formatting if UTC+ zones are ever onboarded.
distributionKw = max(base, intermediate, peak) is semantically questionable
domains/cross-domain/src/bill-pdf-view-model.ts
For GDMTH, distribution demand (demanda de distribución) is the billed peak demand — typically `dband.peak`, not `Math.max(dband.base, dband.intermediate, dband.peak)`. While peak is usually highest, this is not type-enforced. A data anomaly where base > peak would display the wrong value. Pre-existing in the old handler but now consolidated into the canonical mapper — worth fixing here.
localMidnightUtc fixpoint may not converge on fall-back (autumn DST) transitions
domains/cross-domain/src/bill-charts-from-series.ts
The fixpoint loop `g = wall - tzOffMs(new Date(g), tz)` converges for spring-forward gaps (one UTC instant maps to midnight). For fall-back transitions, the same wall-clock midnight maps to two UTC instants. Intl.DateTimeFormat may return either offset, causing the loop to oscillate rather than converge, or settle on the summer occurrence. The `if (n === g) break` guard requires strict equality — on an oscillating pair this exits after 4 iterations at a potentially wrong value. A post-convergence verification step would make this robust.
dayKeys next-day stepping via Date.UTC calendar arithmetic is correct but fragile
domains/cross-domain/src/bill-charts-from-series.ts
`Date.UTC(+c.year, +c.month - 1, +c.day + 1)` normalizes rollover correctly. Correctness depends on `utcToLocalComponents` returning LOCAL calendar components (not UTC). If `utcToLocalComponents` were ever to return UTC components, the timezone offset would be double-counted. The function name is unambiguous but a comment confirming it returns local components would make the invariant explicit.
tco2e uses subtotal (pre-tax) rather than kWh-based emissions factor
domains/cross-domain/src/bill-pdf-view-model.ts
Pre-existing behavior preserved from both callers. CO2e proportional to monetary subtotal rather than kWh consumed means emissions change with tariff rate changes even for identical consumption. Fine for display purposes but worth documenting as intentional.
Empty schedules array silently downgrades to flat chart
domains/cross-domain/src/bill-charts-from-series.ts
`hasSchedules = !!input.schedules && input.schedules.length > 0` — an empty array forces the flat path. If a caller passes `schedules: []` for a GDMTH site (e.g. a hydration bug), the TOU chart is silently skipped rather than flagged. The defensive fallback is correct but the silent degradation could mask upstream data pipeline bugs.
security3
Service-role Lambda intentionally bypasses RLS
services/billing/src/handlers/generate-bill-pdf.handler.ts
Confirmed by code comment and design. The trust boundary is the SFN state machine's IAM role. No code change needed — noting for awareness that any future user-supplied input to this Lambda must go through validation before it reaches the DB layer.
Pure mapper receives pre-resolved domain objects — no re-validation
domains/cross-domain/src/bill-pdf-view-model.ts
toBillPdfViewModel and buildBillPdfRichMeta are pure functions that accept already-resolved domain objects. Security depends entirely on callers enforcing auth (HTTP handler via createRLSDb/withAuth, Lambda via service-role). Correct for the FCIS pattern. Any future caller must enforce its own auth boundary before invoking the mapper.
CO2E_PER_MXN constant exported from shared module
domains/cross-domain/src/bill-pdf-view-model.ts
Harmless. If this factor ever becomes jurisdiction-specific or regulation-driven, migrate it to reference data. No immediate risk.
conventions4
CONCEPT_META value import from renderer leaf package into domain layer
domains/cross-domain/src/bill-pdf-view-model.ts:12
CONCEPT_META is a runtime value (Record<string,{label,unit}>) imported from @batu/bill-pdf/view-model into cross-domain. The rule explicitly allows the view-model subpath (no react-pdf pull-in) and the file comment explains the rationale. The concern is that CONCEPT_META is display metadata conceptually belonging to the rendering contract. If concept labels ever need to differ by locale or renderer, this import becomes load-bearing coupling. Consider passing CONCEPT_META as a parameter to toBillPdfViewModel for better inversion of control.
Optional-field spread pattern in toBillPdfViewModel is verbose
domains/cross-domain/src/bill-pdf-view-model.ts
~12 instances of `...(meta.X !== undefined && { X: meta.X })`. Not a convention violation (explicit per-key, not blind spread), but a `stripUndefined` helper would reduce boilerplate and make future field additions less error-prone.
Comment on optional-field block partially explains WHAT rather than WHY
domains/cross-domain/src/bill-pdf-view-model.ts
`// Rich parity fields — only spread when supplied (absent ⇒ minimal layout).` describes the mechanism (WHAT) alongside the constraint (WHY). The minimal-layout fallback IS the non-obvious constraint, so the comment is partially compliant — just verbose.
fmtBillDateDDMMYYYY exported alongside coordinator shells in cross-domain barrel
domains/cross-domain/src/index.ts
A pure formatting utility mixed into the coordinator barrel is fine per current rules. Future consideration: if formatters accumulate, a dedicated @batu/date-utils package would be a cleaner home.
tests8
toBillPdfViewModel TOU and flat paths have no unit tests
domains/cross-domain/src/bill-pdf-view-model.ts
The new canonical view-model mapper has two distinct code paths (TOU band vs flat energy consumption). Neither path is tested. A regression would produce silently wrong PDF output — wrong kWh totals, wrong band labels — for all customers. The function is pure; tests are trivial to add and should be a gate for merging.
Tijuana fall-back (autumn DST transition) not tested in bill-charts-from-series
domains/cross-domain/src/__tests__/bill-charts-from-series.test.ts
Spring-forward (Mar 8 Tijuana) is tested but the autumn fall-back (Nov 1 2026 Tijuana, 25-hour day) is not. The fall-back exercises a different code path in localMidnightUtc and the day-step loop. An off-by-one in the 25-hour day would produce wrong kWh totals for Tijuana customers in November billing periods. Tijuana is an active production timezone.
calendarMonthly series guard in deriveBillChartsFromSeries is untested
domains/cross-domain/src/__tests__/bill-charts-from-series.test.ts
The guard `if (demand.calendarMonthly) return empty structure` prevents 1M (monthly-granularity) series from being split into per-day TOU bands. An uncovered guard means if the guard were accidentally removed, a 1M-source site would produce silently incorrect TOU charts (entire month dumped into base band at month-start midnight). One test with `demand = { ..., calendarMonthly: true }` covers this.
buildBillPdfRichMeta null-ctx / null-siteMeta paths untested
domains/cross-domain/src/bill-pdf-view-model.ts
The minimal path (ctx=null, siteMeta=null) produces a header with only `rpu`, `dueDateLabel`, and `tco2e`. If a null coalesce is wrong, optional chaining returns `undefined` for a required field, which could crash the React-PDF renderer at document generation time. A targeted test covering this path costs nothing.
localMidnightUtc convergence not tested across multiple IANA zones
domains/cross-domain/src/__tests__/bill-charts-from-series.test.ts
The fixpoint iteration is tested only via the Tijuana spring-forward case. CDMX (UTC-6, no DST) should converge in one iteration — a test confirming it would guard against regressions where the loop adds unnecessary iterations or fails on the identity case (zero DST shift).
generate-bill-pdf handler wiring not tested after mapper refactor
services/billing/src/handlers/generate-bill-pdf.handler.ts
The handler was refactored to use the new shared mapper. No test verifies that the correct mapper is called, that its output flows to the PDF renderer, or that the `yearMonth` and `generatedAt` fields are correctly wired. The `?? null` coercion fix for ctx/siteMeta is also unguarded by a test.
fmtBillDateDDMMYYYY Dec 31 / Feb 29 boundary cases uncovered
domains/cross-domain/src/bill-pdf-view-model.ts
Though the function is simple UTC formatting, bill periods around year-end and leap-year February are exactly where off-by-one-day errors appear. Two-line tests would eliminate any doubt.
Empty-schedules flat path lacks energy-conservation assertion
domains/cross-domain/src/__tests__/bill-charts-from-series.test.ts
The test for `schedules: []` checks only that the function runs and returns the flat path. The TOU tests explicitly verify Σ(bands) == flat total (the energy-conservation invariant). Adding the same assertion to the flat-path test makes coverage symmetric.
improvement6
fmtBillDateDDMMYYYY precondition needs documentation
domains/cross-domain/src/bill-pdf-view-model.ts
The function uses UTC components, which is correct only when the Date is a local-midnight-as-UTC value (valid for all current Mexican production sites UTC-6/7/8). A JSDoc stating this precondition (`@param d — must be a UTC timestamp representing local midnight in the site's timezone`) would prevent misuse if UTC+ zones are onboarded. Without it, callers may pass a `Date` that looks correct but gives wrong day in UTC+.
Optional-field spreading in toBillPdfViewModel would benefit from a stripUndefined helper
domains/cross-domain/src/bill-pdf-view-model.ts
12 conditional spreads `...(meta.X !== undefined && { X: meta.X })` is verbose and a typo silently drops a field. A `stripUndefined<T extends object>(o: T): Partial<T>` utility (or Object.fromEntries filter) would halve the boilerplate and be reusable by future view-model builders in the codebase.
localMidnightUtc fixpoint loop needs a comment explaining DST convergence guarantee
domains/cross-domain/src/bill-charts-from-series.ts
The loop (up to 4 iterations) is non-obvious. A two-line comment explaining that DST shifts are at most ±1h so one correction overshoots by at most 1h and a second pass lands exactly would prevent future 'simplification' that removes the loop.
buildBillPdfRichMeta opts parameter could have a more descriptive name
domains/cross-domain/src/bill-pdf-view-model.ts
Renamed from `body: { rpu?: string }` to `opts?: { readonly rpu?: string }`. The generic `opts` name loses the traceability of the HTTP request body origin. `billPdfOpts` or `overrides` would be clearer in call-site grep.
CHART_W = 480 has no derivation comment
packages/bill-pdf/src/bill-document.tsx
The comment explains the fix but not how 480 was derived (A4 595.28pt − 2×34pt padding = 527.28pt; TOU layout uses 2×22pt Y-axes = 524pt; PDBT uses 1×22pt = 502pt; 480 fits both). Pinning this to the page spec would prevent drift in a future layout change.
dayKeys helper could be extracted as a standalone testable function
domains/cross-domain/src/bill-charts-from-series.ts
The `dayKeys` generation loop is pure and has DST edge cases. Currently embedded in `deriveBillChartsFromSeries`. Extracting it as a named export would allow direct unit testing of the day-stepping logic without constructing full chart inputs.