mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-14 14:40:52 +00:00
The guard tested whether the argument list contained the substring "timeZone".
`timeZoneName: "short"` contains it. That option appends "GMT+1" to the output
and leaves the zone as the browser's, so a formatter written that way sailed
through while being exactly as wrong as a bare one — and looking more careful
than a bare one, which is worse.
Tightening it to a colon-terminated property then reported nine correct call
sites, because the shorthand `{ timeZone }` is what most of this tree writes.
The honest response to that would have been to allowlist all nine, which is
how a guard stops guarding. It accepts both forms now, and a test pins both
directions rather than leaving the regex to be trusted.
Two exclusion reasons in the backup plan aged inside their own release and are
corrected: push attempts now also hold the reminder dedup anchors this release
introduced, and the thumbnail's "rebuilds from the stored bytes" only holds on
the disaster-recovery path, since a portable export carries no bytes to rebuild
from.
186 lines
7.5 KiB
TypeScript
186 lines
7.5 KiB
TypeScript
import { readFileSync } from "node:fs";
|
|
import { execFileSync } from "node:child_process";
|
|
import { resolve } from "node:path";
|
|
|
|
import { describe, expect, it } from "vitest";
|
|
|
|
/**
|
|
* A formatter that renders an instant must say which calendar it renders in.
|
|
*
|
|
* `new Intl.DateTimeFormat(locale, {…})` and `date.toLocaleDateString(…)` fall
|
|
* back to the BROWSER's timezone when none is given. That is a different
|
|
* question from the one this app answers: a reading belongs to the day it was
|
|
* where the person lives, not where their laptop happens to be. A reporter hit
|
|
* exactly this (#490), and the sweep behind it found four separate surfaces
|
|
* that had drifted apart — one of them printed a shared report a day off from
|
|
* the page it was downloaded from.
|
|
*
|
|
* The check is a source-shape check, and it is deliberately coarse in one
|
|
* direction: it cannot tell a Date built from calendar arithmetic (a month
|
|
* label, a weekday header — no instant involved, no timezone question) from a
|
|
* Date parsed out of stored data. So those legitimately-bare sites live on the
|
|
* allowlist with a reason, and the guard's real job is the ratchet: a NEW bare
|
|
* formatter has to be argued for in writing before it can ship.
|
|
*
|
|
* `.toLocaleString(` is deliberately NOT matched. It is overwhelmingly a
|
|
* number formatter in this tree, and flagging thousands separators would bury
|
|
* the four real hits in noise until someone silenced the whole guard.
|
|
*/
|
|
|
|
const REPO_ROOT = resolve(__dirname, "../..");
|
|
|
|
/**
|
|
* Bare formatter sites that are correct as they stand. Keep this short: it is
|
|
* an inventory of exceptions, not a parking space. Every entry says why.
|
|
*/
|
|
const ALLOWLIST: ReadonlyArray<{ file: string; why: string }> = [
|
|
{
|
|
file: "src/components/ui/calendar.tsx",
|
|
why: "A `data-day` attribute on a calendar cell, derived from the grid's own arithmetic. It identifies a cell, it does not render a recorded moment.",
|
|
},
|
|
{
|
|
file: "src/components/cycle/cycle-calendar.tsx",
|
|
why: "Weekday headers walked from a fixed local date, and a month label built from a month anchor. Both are calendar arithmetic with no instant behind them.",
|
|
},
|
|
{
|
|
file: "src/lib/intl/formatter-cache.ts",
|
|
why: "The shared cache itself. Its options come from the caller, so the timezone is the caller's to supply and the guard should be asking them, not this.",
|
|
},
|
|
];
|
|
|
|
const FORMATTER_PATTERNS = [
|
|
/new Intl\.DateTimeFormat\s*\(/g,
|
|
/\.toLocale(?:Date|Time)String\s*\(/g,
|
|
];
|
|
|
|
/** The argument list starting at the `(` at or after `from`. */
|
|
function argumentList(source: string, from: number): string {
|
|
const open = source.indexOf("(", from);
|
|
if (open === -1) return "";
|
|
let depth = 0;
|
|
for (let i = open; i < source.length; i += 1) {
|
|
if (source[i] === "(") depth += 1;
|
|
else if (source[i] === ")") {
|
|
depth -= 1;
|
|
if (depth === 0) return source.slice(open, i + 1);
|
|
}
|
|
}
|
|
return source.slice(open);
|
|
}
|
|
|
|
function bareFormatterSites(): Array<{ file: string; line: number }> {
|
|
const listed = execFileSync(
|
|
"grep",
|
|
[
|
|
"-rlE",
|
|
"new Intl\\.DateTimeFormat|\\.toLocale(Date|Time)String\\(",
|
|
"src",
|
|
"--include=*.ts",
|
|
"--include=*.tsx",
|
|
],
|
|
{ cwd: REPO_ROOT, encoding: "utf8" },
|
|
)
|
|
.trim()
|
|
.split("\n")
|
|
.filter(Boolean)
|
|
// Tests pin instants on purpose and are not user-facing surfaces.
|
|
.filter((f) => !f.includes("__tests__"));
|
|
|
|
const hits: Array<{ file: string; line: number }> = [];
|
|
for (const file of listed) {
|
|
const source = readFileSync(resolve(REPO_ROOT, file), "utf8");
|
|
for (const pattern of FORMATTER_PATTERNS) {
|
|
pattern.lastIndex = 0;
|
|
for (let m = pattern.exec(source); m !== null; m = pattern.exec(source)) {
|
|
const args = argumentList(source, m.index + m[0].length - 1);
|
|
// The property, not the substring, and both ways of writing it.
|
|
//
|
|
// `timeZoneName: "short"` is a DISPLAY option — it appends "GMT+1" to
|
|
// the output and leaves the zone as the browser's. A plain substring
|
|
// check waves it through, and the result then looks MORE
|
|
// timezone-aware than a bare formatter while being exactly as wrong.
|
|
//
|
|
// The shorthand `{ timeZone }` is the common form in this tree (nine
|
|
// correct call sites use it), so a colon-only check would report all
|
|
// of them and the honest response would be to allowlist them — which
|
|
// is how a guard stops guarding.
|
|
if (/\btimeZone\s*(?::|,|\}|$)/m.test(args)) continue;
|
|
hits.push({
|
|
file,
|
|
line: source.slice(0, m.index).split("\n").length,
|
|
});
|
|
}
|
|
}
|
|
}
|
|
return hits;
|
|
}
|
|
|
|
describe("date formatters name the calendar they render in", () => {
|
|
const hits = bareFormatterSites();
|
|
|
|
it("finds formatter sites at all (the guard must not pass vacuously)", () => {
|
|
// If this drops to zero the grep has stopped matching and every assertion
|
|
// below became free.
|
|
const all = execFileSync(
|
|
"grep",
|
|
[
|
|
"-rlE",
|
|
"new Intl\\.DateTimeFormat|\\.toLocale(Date|Time)String\\(",
|
|
"src",
|
|
"--include=*.ts",
|
|
"--include=*.tsx",
|
|
],
|
|
{ cwd: REPO_ROOT, encoding: "utf8" },
|
|
)
|
|
.trim()
|
|
.split("\n")
|
|
.filter(Boolean);
|
|
expect(all.length).toBeGreaterThan(5);
|
|
});
|
|
|
|
it("has no unexplained formatter without an explicit timeZone", () => {
|
|
const unexplained = hits.filter(
|
|
(h) => !ALLOWLIST.some((a) => a.file === h.file),
|
|
);
|
|
if (unexplained.length > 0) {
|
|
throw new Error(
|
|
`${unexplained.length} formatter call(s) render a date without naming a timezone:\n\n` +
|
|
unexplained.map((h) => ` ${h.file}:${h.line}`).join("\n") +
|
|
"\n\nWithout `timeZone` these follow the browser, not the profile. Pass the " +
|
|
"profile timezone (`readStoredTimezone() || DEFAULT_TIMEZONE` on the client, " +
|
|
"`resolveUserTimezone(userId)` on the server), or use the shared helpers in " +
|
|
"@/lib/format. If the value is calendar arithmetic rather than a recorded " +
|
|
"instant, add the file to ALLOWLIST in this test with the reason.",
|
|
);
|
|
}
|
|
});
|
|
|
|
it("does not accept timeZoneName as a substitute for timeZone", () => {
|
|
// `timeZoneName: "short"` appends "GMT+1" to the output and leaves the
|
|
// zone as the browser's. The first version of this guard tested for the
|
|
// substring "timeZone", which that satisfies — so a formatter that LOOKS
|
|
// more timezone-aware than a bare one sailed through while being exactly
|
|
// as wrong. An independent read found it; this pins it.
|
|
const property = /\btimeZone\s*(?::|,|\}|$)/m;
|
|
expect(property.test('{ hour: "2-digit", timeZoneName: "short" }')).toBe(
|
|
false,
|
|
);
|
|
expect(property.test('{ hour: "2-digit", timeZone: tz }')).toBe(true);
|
|
expect(property.test("{ timeZone : tz }")).toBe(true);
|
|
// The shorthand forms, which are what most of this tree actually writes.
|
|
expect(property.test("{ timeZone, year: 'numeric' }")).toBe(true);
|
|
expect(property.test("{ ...options, timeZone }")).toBe(true);
|
|
});
|
|
|
|
it("carries no allowlist entry that no longer has a bare formatter", () => {
|
|
// A stale exception is worse than none: it documents a decision about code
|
|
// that has moved on, and it quietly covers whatever lands in that file next.
|
|
for (const entry of ALLOWLIST) {
|
|
expect(
|
|
hits.some((h) => h.file === entry.file),
|
|
`stale allowlist entry — ${entry.file} no longer has a bare formatter`,
|
|
).toBe(true);
|
|
}
|
|
});
|
|
});
|