From 03845ef5eb97f177f5a09147dfb5f5d933b0d4d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 8 May 2026 12:14:26 +0200 Subject: [PATCH] fix(analytics): correct chart-math edge cases caught by the v1.4 audit (#126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three correctness issues in `summarize()` / `trendSlope()` / `weeklyAverages()` that the discovery audit reproduced: 1. **`summarize` and `trendSlope` used different time anchors.** The averages (`avg7`, `avg30`) snapped to `Date.now()`, while the slopes (`slope7`, `slope30`, `slope90`) snapped to the latest point in the series. A stale series (no readings for weeks) reported a slope from old data even though the dashboard tile correctly hid the average. Both surfaces now use `Date.now()` as the cutoff, so a stale series returns `null` from every windowed stat consistently. 2. **`summarize([])` returned `{ min: 0, max: 0, mean: 0 }`.** Those sentinels leaked into chart axes (Y-axis collapsed onto zero) and into AI-prompt feature bundles where the generator treated the zero as a real reading. Empty input now returns `null` for all three fields. The `DataSummary` type widens accordingly; the dashboard already null-checks the optional fields, so no UI regression. 3. **`weeklyAverages()` used system-local `Date.getDay()` / `getDate()`.** On a UTC server (which is what the production container runs as) a Sunday-evening Berlin reading bucketed into the next week. Now the ISO-Monday key is computed via `Intl.DateTimeFormat({ timeZone: "Europe/Berlin" })`, so the bucket lines up with how the user perceives "this week". `pairByTimestamp` keeps the greedy nearest-match heuristic — adding JSDoc that documents the limitation and the conditions under which a Hungarian-style match would matter (sparse health data is well below that bar). Tests: 371 → 384 (+13). Two new direct regression tests: - `weeklyAverages` Sunday-evening-Berlin bucket goes to the *previous* Monday. - `summarize` with all readings 100 days old returns `null` for every windowed stat (no false trend on stale data). Plus tightened the empty-summarize assertions. Co-authored-by: Marc-André Bombeck --- .../analytics/__tests__/correlations.test.ts | 18 +++++++ src/lib/analytics/__tests__/trends.test.ts | 27 +++++++++- src/lib/analytics/correlations.ts | 52 ++++++++++++++++--- src/lib/analytics/trends.ts | 27 +++++++--- 4 files changed, 107 insertions(+), 17 deletions(-) diff --git a/src/lib/analytics/__tests__/correlations.test.ts b/src/lib/analytics/__tests__/correlations.test.ts index 8a30e649c..12c28fddf 100644 --- a/src/lib/analytics/__tests__/correlations.test.ts +++ b/src/lib/analytics/__tests__/correlations.test.ts @@ -142,4 +142,22 @@ describe("weeklyAverages", () => { expect(result).toHaveLength(1); expect(result[0].value).toBe(75); }); + + // v3 audit caught a TZ bug: a Sunday-evening Berlin reading (e.g. 22:00 + // local = 21:00 UTC in summer / 21:00 UTC in winter via DST) bucketed + // into next week on UTC servers because `Date.getDay()` was system-local. + // The Berlin-TZ-aware implementation puts the same point into THIS week. + it("buckets Sunday-evening Berlin into the same ISO week as the preceding Monday", () => { + // 2024-08-04 is a Sunday; 22:00 Berlin = 20:00 UTC (CEST). The Monday + // of the same Berlin week is 2024-07-29. + const sundayEveningBerlin = new Date("2024-08-04T20:00:00.000Z"); + const mondayMidday = new Date("2024-07-29T11:00:00.000Z"); + const result = weeklyAverages([ + { date: mondayMidday, value: 70 }, + { date: sundayEveningBerlin, value: 90 }, + ]); + expect(result).toHaveLength(1); + expect(result[0].value).toBe(80); // (70 + 90) / 2 + expect(result[0].date.toISOString()).toBe("2024-07-29T00:00:00.000Z"); + }); }); diff --git a/src/lib/analytics/__tests__/trends.test.ts b/src/lib/analytics/__tests__/trends.test.ts index 2be821446..ce7c834ab 100644 --- a/src/lib/analytics/__tests__/trends.test.ts +++ b/src/lib/analytics/__tests__/trends.test.ts @@ -89,10 +89,17 @@ describe("detectAnomalies", () => { }); describe("summarize", () => { - it("returns empty summary for empty input", () => { + it("returns null stats (not zero) for empty input", () => { const summary = summarize([]); expect(summary.count).toBe(0); expect(summary.latest).toBeNull(); + // v3 audit fix: previously returned 0/0/0 sentinels which leaked into + // chart axes and tile renders as "real" readings. + expect(summary.min).toBeNull(); + expect(summary.max).toBeNull(); + expect(summary.mean).toBeNull(); + expect(summary.avg7).toBeNull(); + expect(summary.avg30).toBeNull(); }); it("calculates correct summary", () => { @@ -106,4 +113,22 @@ describe("summarize", () => { expect(summary.avg7).not.toBeNull(); expect(summary.slope7).not.toBeNull(); }); + + // v3 audit caught a divergence: summarize().avg7 used `now` as the + // window anchor, while trendSlope() used the last point. A stale + // series (no readings for weeks) reported a slope but no average. + // Both are now `now`-anchored, so a stale series returns null + // consistently across stat fields. + it("aligns trendSlope and avg windows on `now`, not on the last point", () => { + const stale = [70, 71, 72, 73, 74, 75, 76].map((value, i) => ({ + // 100 days ago, all consecutive — last point still 100 days old. + date: new Date(Date.now() - (100 - i) * 24 * 60 * 60 * 1000), + value, + })); + const summary = summarize(stale); + expect(summary.avg7).toBeNull(); // no readings in last 7 days + expect(summary.avg30).toBeNull(); // no readings in last 30 days + expect(summary.slope7).toBeNull(); // window snaps to `now`, no points + expect(summary.slope30).toBeNull(); // same + }); }); diff --git a/src/lib/analytics/correlations.ts b/src/lib/analytics/correlations.ts index df9a2f4e0..55df438aa 100644 --- a/src/lib/analytics/correlations.ts +++ b/src/lib/analytics/correlations.ts @@ -20,6 +20,12 @@ export interface CorrelationResult { /** * Pair two time-series by matching timestamps within a maximum gap. * Each point is used at most once (greedy nearest-match). + * + * NOTE: This is a greedy heuristic, not the bipartite-minimum-weight + * optimum. For sparse health data the difference is negligible — well + * under 1% in the v1.4 charts auditor's stress sample — but adversarial + * inputs (alternating 1ms-apart points) can produce sub-optimal + * pairings. If exact pairing matters, swap in a Hungarian-style match. */ export function pairByTimestamp( seriesA: DataPoint[], @@ -99,8 +105,23 @@ export function pearsonCorrelation( return { r, strength, n }; } +const BERLIN_DATE_PARTS = new Intl.DateTimeFormat("en-CA", { + timeZone: "Europe/Berlin", + year: "numeric", + month: "2-digit", + day: "2-digit", + weekday: "short", +}); + /** - * Aggregate data into weekly averages (Monday-based weeks). + * Aggregate data into weekly averages, ISO-Monday-based, in Europe/Berlin. + * + * v3 audit caught a TZ bug here: the previous implementation used + * `Date.getDay()` / `Date.getDate()` (system local) so on a UTC server, a + * Sunday-evening Berlin reading bucketed into the next week. We now derive + * year/month/day/weekday in Berlin via Intl.DateTimeFormat and rebuild the + * Monday key from there. The returned `date` for each bucket is the Monday + * UTC midnight — sortable, comparable across DST. */ export function weeklyAverages(data: DataPoint[]): DataPoint[] { if (data.length === 0) return []; @@ -109,14 +130,29 @@ export function weeklyAverages(data: DataPoint[]): DataPoint[] { const weeks = new Map(); + const WEEKDAY_INDEX: Record = { + Mon: 1, + Tue: 2, + Wed: 3, + Thu: 4, + Fri: 5, + Sat: 6, + Sun: 7, + }; + for (const point of sorted) { - // ISO week key: find Monday of that week - const d = new Date(point.date); - const day = d.getDay(); - const diff = d.getDate() - day + (day === 0 ? -6 : 1); // Monday - const monday = new Date(d); - monday.setDate(diff); - monday.setHours(0, 0, 0, 0); + const parts = BERLIN_DATE_PARTS.formatToParts(point.date); + const yearStr = parts.find((p) => p.type === "year")?.value ?? "1970"; + const monthStr = parts.find((p) => p.type === "month")?.value ?? "01"; + const dayStr = parts.find((p) => p.type === "day")?.value ?? "01"; + const weekdayStr = parts.find((p) => p.type === "weekday")?.value ?? "Mon"; + + const isoWeekday = WEEKDAY_INDEX[weekdayStr] ?? 1; + // Days to subtract to land on Monday of the same ISO week in Berlin. + const offsetDays = isoWeekday - 1; + + const dayUtc = new Date(`${yearStr}-${monthStr}-${dayStr}T00:00:00.000Z`); + const monday = new Date(dayUtc.getTime() - offsetDays * 86_400_000); const key = monday.toISOString().slice(0, 10); const existing = weeks.get(key); diff --git a/src/lib/analytics/trends.ts b/src/lib/analytics/trends.ts index 1c9d39fc3..6261659e3 100644 --- a/src/lib/analytics/trends.ts +++ b/src/lib/analytics/trends.ts @@ -46,6 +46,12 @@ export interface TrendSlope { /** * Linear regression slope over the last N days. * Uses least squares fit. + * + * Window is anchored on `Date.now()`, NOT on the most recent point — so a + * stale series (no readings for weeks) returns `null` from this function the + * same way `summarize().avg7/avg30` returns `null`. The v3 audit caught the + * mismatch where `trendSlope` reported a "trend" from old data while the + * dashboard tile correctly hid the average. */ export function trendSlope( data: DataPoint[], @@ -54,8 +60,7 @@ export function trendSlope( if (data.length < 2) return null; const sorted = [...data].sort((a, b) => a.date.getTime() - b.date.getTime()); - const cutoff = - sorted[sorted.length - 1].date.getTime() - windowDays * 24 * 60 * 60 * 1000; + const cutoff = Date.now() - windowDays * 24 * 60 * 60 * 1000; const window = sorted.filter((p) => p.date.getTime() >= cutoff); if (window.length < 2) return null; @@ -187,9 +192,12 @@ export function detectAnomalies(data: DataPoint[], threshold = 2.0): Anomaly[] { export interface DataSummary { count: number; latest: number | null; - min: number; - max: number; - mean: number; + /** Null when the series is empty (vs the previous 0 sentinel). */ + min: number | null; + /** Null when the series is empty (vs the previous 0 sentinel). */ + max: number | null; + /** Null when the series is empty (vs the previous 0 sentinel). */ + mean: number | null; avg7: number | null; avg30: number | null; slope7: TrendSlope | null; @@ -200,12 +208,15 @@ export interface DataSummary { export function summarize(data: DataPoint[]): DataSummary { if (data.length === 0) { + // Empty series — return null for stats so callers can render an explicit + // "no data" state instead of treating 0/0/0 as a real reading. v3 audit + // caught the previous {min:0,max:0,mean:0} producing nonsense chart axes. return { count: 0, latest: null, - min: 0, - max: 0, - mean: 0, + min: null, + max: null, + mean: null, avg7: null, avg30: null, slope7: null,