fix(reports): StrictMode-safe period sync + review polish (#253)
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) <noreply@anthropic.com>
This commit is contained in:
parent
f45845e408
commit
d57b2563af
6 changed files with 93 additions and 30 deletions
|
|
@ -32,14 +32,6 @@ export default function ComparePeriodChart({
|
||||||
}: ComparePeriodChartProps) {
|
}: ComparePeriodChartProps) {
|
||||||
const { t, i18n } = useTranslation();
|
const { t, i18n } = useTranslation();
|
||||||
|
|
||||||
if (rows.length === 0) {
|
|
||||||
return (
|
|
||||||
<div className="bg-[var(--card)] border border-[var(--border)] rounded-xl p-6 text-center text-[var(--muted-foreground)] italic">
|
|
||||||
{t("reports.empty.noData")}
|
|
||||||
</div>
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
// The chart stays a spending view: the income-statement result (revenues +
|
// The chart stays a spending view: the income-statement result (revenues +
|
||||||
// result lines, Issue #253) lives in the table, so keep only expense leaves
|
// 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
|
// here — mixing revenue bars into the same axis would misread. Drop subtotal
|
||||||
|
|
@ -56,6 +48,16 @@ export default function ComparePeriodChart({
|
||||||
color: r.categoryColor,
|
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 (
|
||||||
|
<div className="bg-[var(--card)] border border-[var(--border)] rounded-xl p-6 text-center text-[var(--muted-foreground)] italic">
|
||||||
|
{t("reports.empty.noData")}
|
||||||
|
</div>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
const previousFill = "var(--muted-foreground)";
|
const previousFill = "var(--muted-foreground)";
|
||||||
const currentFill = "var(--primary)";
|
const currentFill = "var(--primary)";
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -379,7 +379,7 @@ export default function ComparePeriodTable({
|
||||||
{/* Operating result (revenues − expenses), shown before the
|
{/* Operating result (revenues − expenses), shown before the
|
||||||
transfers section only when transfers exist — otherwise it
|
transfers section only when transfers exist — otherwise it
|
||||||
equals the net total below and would just be noise. */}
|
equals the net total below and would just be noise. */}
|
||||||
{transferSection &&
|
{results.hasTransfers &&
|
||||||
renderResultRow(
|
renderResultRow(
|
||||||
"reports.compare.resultBeforeTransfers",
|
"reports.compare.resultBeforeTransfers",
|
||||||
results.resultBefore,
|
results.resultBefore,
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,10 @@
|
||||||
import { describe, it, expect } from "vitest";
|
import { describe, it, expect } from "vitest";
|
||||||
import { previousMonth, defaultReferencePeriod, comparisonMeta } from "./useCompare";
|
import {
|
||||||
|
previousMonth,
|
||||||
|
defaultReferencePeriod,
|
||||||
|
comparisonMeta,
|
||||||
|
syncReferenceOnPeriodChange,
|
||||||
|
} from "./useCompare";
|
||||||
|
|
||||||
describe("useCompare helpers", () => {
|
describe("useCompare helpers", () => {
|
||||||
describe("previousMonth", () => {
|
describe("previousMonth", () => {
|
||||||
|
|
@ -44,4 +49,40 @@ describe("useCompare helpers", () => {
|
||||||
expect(comparisonMeta("yoy", 2026, 1)).toEqual({ previousYear: 2025, previousMonth: 1 });
|
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");
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -60,6 +60,32 @@ export function comparisonMeta(
|
||||||
return { previousYear: year - 1, previousMonth: month };
|
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 defaultRef = defaultReferencePeriod();
|
||||||
const initialState: State = {
|
const initialState: State = {
|
||||||
mode: "actual",
|
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);
|
||||||
}, [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`.
|
// Keep the reference month in sync with the URL period when the user navigates
|
||||||
// The explicit dropdown remains the primary selector — this effect only
|
// via PeriodSelector — but not on mount. The ref is seeded with the initial
|
||||||
// keeps the two in sync when the user navigates via PeriodSelector.
|
// `to`, so syncReferenceOnPeriodChange only fires on an actual change of `to`,
|
||||||
//
|
// leaving the previous-month default from initialState intact (Issue #253) and
|
||||||
// Skip the FIRST run (mount): useReportsPeriod defaults to the civil-year
|
// staying idempotent under StrictMode's dev double-invoke.
|
||||||
// range, whose `to` (Dec 31) would otherwise snap the reference month to a
|
const lastSyncedToRef = useRef(to);
|
||||||
// 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);
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!didInitPeriodSync.current) {
|
const { lastSyncedTo, next } = syncReferenceOnPeriodChange(
|
||||||
didInitPeriodSync.current = true;
|
lastSyncedToRef.current,
|
||||||
return;
|
to,
|
||||||
}
|
state.year,
|
||||||
const [y, m] = to.split("-").map(Number);
|
state.month,
|
||||||
if (!Number.isFinite(y) || !Number.isFinite(m)) return;
|
);
|
||||||
if (y !== state.year || m !== state.month) {
|
lastSyncedToRef.current = lastSyncedTo;
|
||||||
dispatch({ type: "SET_REFERENCE_PERIOD", payload: { year: y, month: m } });
|
if (next) dispatch({ type: "SET_REFERENCE_PERIOD", payload: next });
|
||||||
}
|
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||||
}, [to]);
|
}, [to]);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -427,7 +427,6 @@
|
||||||
"referenceMonth": "Reference month",
|
"referenceMonth": "Reference month",
|
||||||
"currentAmount": "Current",
|
"currentAmount": "Current",
|
||||||
"previousAmount": "Previous",
|
"previousAmount": "Previous",
|
||||||
"totalRow": "Total",
|
|
||||||
"sections": {
|
"sections": {
|
||||||
"expenses": "Expenses",
|
"expenses": "Expenses",
|
||||||
"income": "Income",
|
"income": "Income",
|
||||||
|
|
|
||||||
|
|
@ -427,7 +427,6 @@
|
||||||
"referenceMonth": "Mois de référence",
|
"referenceMonth": "Mois de référence",
|
||||||
"currentAmount": "Courant",
|
"currentAmount": "Courant",
|
||||||
"previousAmount": "Précédent",
|
"previousAmount": "Précédent",
|
||||||
"totalRow": "Total",
|
|
||||||
"sections": {
|
"sections": {
|
||||||
"expenses": "Dépenses",
|
"expenses": "Dépenses",
|
||||||
"income": "Revenus",
|
"income": "Revenus",
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue