← all branches

feat/billing-improvements

needs attention
f406509 · incrementalpre-PRreviewed 2026-07-30 22:31 UTC5H · 8M · 8L · 5I
The branch
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
The changes (whole branch)
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+291143packages/bill-pdf+218118services/billing+1170apps/platform/src/api+23126infra/cdk+19113pnpm-lock.yaml+210
Blast
27 files, +1102/−351 lines across billing pipeline, CDK infra, and PDF renderer
no PR open — pre-PR branch
ci· No PR open; CI checks not accessible for pre-PR branch via this tokencoderabbit· No .coderabbit.yaml in repo

Findings · 28

correctness7

high

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.

high

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.

medium

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.

medium

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.

medium

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.

low

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.

low

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

info

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.

info

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.

info

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

medium

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.

low

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.

low

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.

info

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

high

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.

high

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.

high

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.

medium

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.

medium

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

medium

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.

low

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.

low

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

medium

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

medium

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.

low

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.

low

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.

low

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.

info

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.

History · 2 commits

  1. f406509needs attentionincremental5H · 8M · 8L2026-07-30 22:31current
  2. 3afe12cneeds attentionincremental1H · 7M · 6L2026-07-30 19:23