From d57b2563af18868e1b1b1103c450609d414ecb7e Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 5 Jul 2026 19:37:22 -0400 Subject: [PATCH] fix(reports): StrictMode-safe period sync + review polish (#253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses /pr-review REQUEST_CHANGES on #255. - useCompare: the "skip first sync" boolean was not StrictMode-safe — the dev double-invoke of effects flipped the flag on setup #1, so setup #2 re-synced the reference month to the civil-year December, re-introducing the very bug Changement 2 fixes (dev only; prod has no double-invoke). Replace it with a value-change guard: a ref seeded with the initial `to` plus a pure syncReferenceOnPeriodChange() that only dispatches when `to` actually changes. Idempotent across the double-invoke, and now unit-tested (5 cases) since the decision is a pure function (the project has no renderHook harness). - Remove the now-orphaned reports.compare.totalRow i18n key (both locales) — the flat grand total it labelled was replaced by the result lines. - ComparePeriodTable: gate the "before transfers" line on results.hasTransfers (previously computed/tested but unused). - ComparePeriodChart: show the no-data empty state when the expense filter leaves nothing (a pure income/transfer period) instead of bare axes. Build + 690 vitest green. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/components/reports/ComparePeriodChart.tsx | 18 +++--- src/components/reports/ComparePeriodTable.tsx | 2 +- src/hooks/useCompare.test.ts | 43 +++++++++++++- src/hooks/useCompare.ts | 58 +++++++++++++------ src/i18n/locales/en.json | 1 - src/i18n/locales/fr.json | 1 - 6 files changed, 93 insertions(+), 30 deletions(-) diff --git a/src/components/reports/ComparePeriodChart.tsx b/src/components/reports/ComparePeriodChart.tsx index 719ae91..290e333 100644 --- a/src/components/reports/ComparePeriodChart.tsx +++ b/src/components/reports/ComparePeriodChart.tsx @@ -32,14 +32,6 @@ export default function ComparePeriodChart({ }: ComparePeriodChartProps) { const { t, i18n } = useTranslation(); - if (rows.length === 0) { - return ( -
- {t("reports.empty.noData")} -
- ); - } - // The chart stays a spending view: the income-statement result (revenues + // result lines, Issue #253) lives in the table, so keep only expense leaves // here — mixing revenue bars into the same axis would misread. Drop subtotal @@ -56,6 +48,16 @@ export default function ComparePeriodChart({ color: r.categoryColor, })); + // Empty when there is no data at all, or when the period is pure + // income/transfers (no expense bars to draw) — avoid rendering bare axes. + if (chartData.length === 0) { + return ( +
+ {t("reports.empty.noData")} +
+ ); + } + const previousFill = "var(--muted-foreground)"; const currentFill = "var(--primary)"; diff --git a/src/components/reports/ComparePeriodTable.tsx b/src/components/reports/ComparePeriodTable.tsx index 24e6236..acb7a01 100644 --- a/src/components/reports/ComparePeriodTable.tsx +++ b/src/components/reports/ComparePeriodTable.tsx @@ -379,7 +379,7 @@ export default function ComparePeriodTable({ {/* Operating result (revenues − expenses), shown before the transfers section only when transfers exist — otherwise it equals the net total below and would just be noise. */} - {transferSection && + {results.hasTransfers && renderResultRow( "reports.compare.resultBeforeTransfers", results.resultBefore, diff --git a/src/hooks/useCompare.test.ts b/src/hooks/useCompare.test.ts index 1258b3f..500d293 100644 --- a/src/hooks/useCompare.test.ts +++ b/src/hooks/useCompare.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest"; -import { previousMonth, defaultReferencePeriod, comparisonMeta } from "./useCompare"; +import { + previousMonth, + defaultReferencePeriod, + comparisonMeta, + syncReferenceOnPeriodChange, +} from "./useCompare"; describe("useCompare helpers", () => { describe("previousMonth", () => { @@ -44,4 +49,40 @@ describe("useCompare helpers", () => { expect(comparisonMeta("yoy", 2026, 1)).toEqual({ previousYear: 2025, previousMonth: 1 }); }); }); + + describe("syncReferenceOnPeriodChange", () => { + // The ref is seeded with the initial `to`, so the mount call has to===last. + it("no-ops on mount, preserving the previous-month default", () => { + const r = syncReferenceOnPeriodChange("2026-12-31", "2026-12-31", 2026, 6); + expect(r.next).toBeNull(); + expect(r.lastSyncedTo).toBe("2026-12-31"); + }); + + // StrictMode double-invokes effects in dev: calling twice with the same `to` + // must not sync (a fire-once boolean would wrongly snap to December on run 2). + it("is idempotent across a repeated `to` (StrictMode-safe)", () => { + const a = syncReferenceOnPeriodChange("2026-12-31", "2026-12-31", 2026, 6); + const b = syncReferenceOnPeriodChange(a.lastSyncedTo, "2026-12-31", 2026, 6); + expect(a.next).toBeNull(); + expect(b.next).toBeNull(); + }); + + it("syncs the reference month when `to` changes (user navigation)", () => { + const r = syncReferenceOnPeriodChange("2026-12-31", "2026-06-30", 2026, 12); + expect(r.next).toEqual({ year: 2026, month: 6 }); + expect(r.lastSyncedTo).toBe("2026-06-30"); + }); + + it("records a changed `to` but does not dispatch when it already matches state", () => { + const r = syncReferenceOnPeriodChange("2026-12-31", "2026-06-30", 2026, 6); + expect(r.next).toBeNull(); + expect(r.lastSyncedTo).toBe("2026-06-30"); + }); + + it("ignores an unparseable `to`", () => { + const r = syncReferenceOnPeriodChange("2026-12-31", "garbage", 2026, 6); + expect(r.next).toBeNull(); + expect(r.lastSyncedTo).toBe("garbage"); + }); + }); }); diff --git a/src/hooks/useCompare.ts b/src/hooks/useCompare.ts index 0055468..23e73f7 100644 --- a/src/hooks/useCompare.ts +++ b/src/hooks/useCompare.ts @@ -60,6 +60,32 @@ export function comparisonMeta( return { previousYear: year - 1, previousMonth: month }; } +/** + * Pure decision for the URL-period → reference-month sync effect. + * + * Gates on a *change* of `to` (the period's upper bound) rather than a + * fire-once flag: seeded with the initial `to`, the mount is a no-op, so the + * previous-month default from initialState survives instead of snapping to the + * civil-year December that useReportsPeriod yields by default (Issue #253). + * Because it keys off a value change, it is idempotent under React StrictMode's + * dev double-invoke of effects — called twice with the same `to` it dispatches + * nothing the second time (a fire-once boolean would wrongly sync on run #2). + * + * Returns the `to` to remember plus an optional reference period to dispatch. + */ +export function syncReferenceOnPeriodChange( + lastSyncedTo: string, + to: string, + currentYear: number, + currentMonth: number, +): { lastSyncedTo: string; next: { year: number; month: number } | null } { + if (to === lastSyncedTo) return { lastSyncedTo, next: null }; + const [y, m] = to.split("-").map(Number); + if (!Number.isFinite(y) || !Number.isFinite(m)) return { lastSyncedTo: to, next: null }; + if (y === currentYear && m === currentMonth) return { lastSyncedTo: to, next: null }; + return { lastSyncedTo: to, next: { year: y, month: m } }; +} + const defaultRef = defaultReferencePeriod(); const initialState: State = { mode: "actual", @@ -119,25 +145,21 @@ export function useCompare() { fetch(state.mode, state.subMode, state.year, state.month); }, [fetch, state.mode, state.subMode, state.year, state.month]); - // When the URL period changes, align the reference month with `to`. - // The explicit dropdown remains the primary selector — this effect only - // keeps the two in sync when the user navigates via PeriodSelector. - // - // Skip the FIRST run (mount): useReportsPeriod defaults to the civil-year - // range, whose `to` (Dec 31) would otherwise snap the reference month to a - // future, empty December and clobber the previous-month default carried by - // initialState (Issue #253). Only user-driven period changes sync afterwards. - const didInitPeriodSync = useRef(false); + // Keep the reference month in sync with the URL period when the user navigates + // via PeriodSelector — but not on mount. The ref is seeded with the initial + // `to`, so syncReferenceOnPeriodChange only fires on an actual change of `to`, + // leaving the previous-month default from initialState intact (Issue #253) and + // staying idempotent under StrictMode's dev double-invoke. + const lastSyncedToRef = useRef(to); useEffect(() => { - if (!didInitPeriodSync.current) { - didInitPeriodSync.current = true; - return; - } - const [y, m] = to.split("-").map(Number); - if (!Number.isFinite(y) || !Number.isFinite(m)) return; - if (y !== state.year || m !== state.month) { - dispatch({ type: "SET_REFERENCE_PERIOD", payload: { year: y, month: m } }); - } + const { lastSyncedTo, next } = syncReferenceOnPeriodChange( + lastSyncedToRef.current, + to, + state.year, + state.month, + ); + lastSyncedToRef.current = lastSyncedTo; + if (next) dispatch({ type: "SET_REFERENCE_PERIOD", payload: next }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [to]); diff --git a/src/i18n/locales/en.json b/src/i18n/locales/en.json index c7f4647..937e58c 100644 --- a/src/i18n/locales/en.json +++ b/src/i18n/locales/en.json @@ -427,7 +427,6 @@ "referenceMonth": "Reference month", "currentAmount": "Current", "previousAmount": "Previous", - "totalRow": "Total", "sections": { "expenses": "Expenses", "income": "Income", diff --git a/src/i18n/locales/fr.json b/src/i18n/locales/fr.json index 93e585e..109f0e3 100644 --- a/src/i18n/locales/fr.json +++ b/src/i18n/locales/fr.json @@ -427,7 +427,6 @@ "referenceMonth": "Mois de référence", "currentAmount": "Courant", "previousAmount": "Précédent", - "totalRow": "Total", "sections": { "expenses": "Dépenses", "income": "Revenus",