diff --git a/docs/TODO.md b/docs/TODO.md index 77c81db..69afe54 100755 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -8,7 +8,7 @@ decisions behind it are in `docs/specs/` — one spec per sub-project, each recording the alternatives that were rejected. Rules that outlived their task moved to `AGENTS.md`. -**Current state:** merged to `main`. 583 tests passing, +**Current state:** merged to `main`. 661 tests passing, `bunx tsc --noEmit` clean, `bun run build` succeeds on both the live-fetch and offline paths. @@ -93,27 +93,7 @@ reason. Fine for one "$" currency; if the owner adds two, typing the code disambiguates and nothing warns them. Tested, documented, not solved. -## 4. Nobody has independently reviewed the rule-history editing surfaces - -Tasks 3-6 of the versioned-allocation-rules work — `setRuleFrom` / -`removeRuleFrom`, the JSON import validation, the Settings rule-history editor, -and the month view's "change from here" — were executed by hand rather than -dispatched, and no second pair of eyes has been over the diff. - -Worth reading first: the four commit messages, `de128e3`, `1cb91d3`, `e3023d0` -and `f94e458`. They record why each surface works the way it does, including -three of the four places the original briefs were wrong — a number field that -could not be cleared, a component that kept the previous post's rule because it -was not keyed, and missing validation that let a NaN allocation into the fold. - -(The fuller task log lived in a tool-managed ledger under `.superpowers/`, which -ignores its own contents and was never part of the repository. This file used to -send readers there, which worked only on the machine that wrote it.) - -Both surfaces have since been seen working in a browser. What is missing is -review, not verification. - -## 5. Still needs a human +## 4. Still needs a human `scripts/screenshot.ts` drives headless Chrome, so this list is much shorter than it was. Already seen and confirmed working: the carry meter (including red diff --git a/src/export/json.test.ts b/src/export/json.test.ts index cd8b2ef..7a75f19 100644 --- a/src/export/json.test.ts +++ b/src/export/json.test.ts @@ -230,6 +230,19 @@ describe("rule versions on import", () => { data.posts[0].rules = [{ from: "2026-01", rule: { kind: "everySecondTuesday" } }]; expect(() => parseDatasetJson(JSON.stringify(data))).toThrow(/everySecondTuesday/); }); + + test.each([ + ["missing percent", { kind: "percentOfIncome" }], + ["string percent", { kind: "percentOfIncome", percent: "10" }], + ["null percent", { kind: "percentOfIncome", percent: null }], + ["missing fixed amount", { kind: "fixed", amount: { currency: "DKK" } }], + ["string fixed amount", { kind: "fixed", amount: { amount: "500", currency: "DKK" } }], + ["null fixed amount", { kind: "fixed", amount: { amount: null, currency: "DKK" } }], + ])("rejects a %s before it can corrupt the fold", (_label, rule) => { + const data = populated() as any; + data.posts[0].rules = [{ from: "2026-01", rule }]; + expect(() => parseDatasetJson(JSON.stringify(data))).toThrow(ImportValidationError); + }); }); describe("the decimal-places setting on import", () => { diff --git a/src/export/json.ts b/src/export/json.ts index 7ae743d..4829bdc 100644 --- a/src/export/json.ts +++ b/src/export/json.ts @@ -301,18 +301,27 @@ export function parseDatasetJson(text: string): Dataset { } seenFrom.add(from); - // The rule's VALUE is deliberately unvalidated — a percentage above 100 - // and a negative amount are both legal. Its SHAPE is not: an unknown - // kind falls through resolveRule's percentage branch and turns every - // figure after it into NaN, and an unsupported currency throws - // MissingRateError deep inside the fold rather than here. + // Rule values may be negative or over 100, but must be finite numbers. + // A missing or non-numeric value can silently turn the fold into NaN. const rule = (version.rule ?? {}) as { kind?: unknown; - amount?: { currency?: unknown }; + amount?: { amount?: unknown; currency?: unknown }; + percent?: unknown; }; if (rule.kind === "fixed") { requireCurrency(rule.amount?.currency, `post "${post.name}" rule from ${from}`, defined); - } else if (rule.kind !== "percentOfIncome") { + if (typeof rule.amount?.amount !== "number" || !Number.isFinite(rule.amount.amount)) { + throw new ImportValidationError( + `Post "${post.name}" has a fixed rule from "${from}" with an amount that is not a finite number`, + ); + } + } else if (rule.kind === "percentOfIncome") { + if (typeof rule.percent !== "number" || !Number.isFinite(rule.percent)) { + throw new ImportValidationError( + `Post "${post.name}" has a percentage rule from "${from}" that is not a finite number`, + ); + } + } else { throw new ImportValidationError( `Post "${post.name}" has a rule from "${from}" of unknown kind "${String(rule.kind)}"`, ); diff --git a/src/ui/components/RuleEditors.test.tsx b/src/ui/components/RuleEditors.test.tsx new file mode 100644 index 0000000..786b240 --- /dev/null +++ b/src/ui/components/RuleEditors.test.tsx @@ -0,0 +1,101 @@ +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +if (!GlobalRegistrator.isRegistered) GlobalRegistrator.register(); +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +import { afterEach, expect, test } from "bun:test"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { createSeedDataset } from "../../domain/seed.ts"; +import { store } from "../../store/index.ts"; +import { RuleHistory } from "./RuleHistory.tsx"; +import { RuleFromMonth } from "../routes/MonthRoute.tsx"; + +const originalMutate = store.mutate; +let root: Root | null = null; +let container: HTMLDivElement | null = null; + +function mount(child: React.ReactNode): HTMLDivElement { + container = document.createElement("div"); + document.body.append(container); + root = createRoot(container); + act(() => root!.render(child)); + return container; +} + +afterEach(() => { + store.mutate = originalMutate; + if (root) act(() => root!.unmount()); + root = null; + container?.remove(); + container = null; +}); + +test("change from here preserves the currency of a prefilled fixed rule", async () => { + const data = createSeedDataset("2026-01"); + const post = data.posts[0]!; + const initial = { kind: "fixed" as const, amount: { amount: 100, currency: "USD" } }; + let saved = false; + store.mutate = async (change) => { + change(data); + saved = true; + }; + const el = mount( + {}} />, + ); + expect(el.querySelector('input[aria-label="Amount in USD"]')).not.toBeNull(); + + await act(async () => { + (Array.from(el.querySelectorAll("button")).find((button) => button.textContent === "Apply")!).click(); + }); + expect(saved).toBe(true); + expect(post.rules[0]!.rule).toEqual(initial); +}); + +test("change from here keeps the editor open and explains a failed write", async () => { + const data = createSeedDataset("2026-01"); + let closed = false; + store.mutate = async () => { + throw new Error("Disk full"); + }; + const el = mount( + { closed = true; }} + />, + ); + + await act(async () => { + (Array.from(el.querySelectorAll("button")).find((button) => button.textContent === "Apply")!).click(); + }); + expect(closed).toBe(false); + expect(el.querySelector('[role="alert"]')?.textContent).toContain("Disk full"); +}); + +test("rule history explains a failed write", async () => { + const data = createSeedDataset("2026-01"); + const post = data.posts[0]!; + post.rules = [{ from: "2026-01", rule: { kind: "percentOfIncome", percent: 10 } }]; + store.mutate = async () => { + throw new Error("Disk full"); + }; + const el = mount(); + + await act(async () => { + (Array.from(el.querySelectorAll("button")).find((button) => button.textContent === "remove")!).click(); + }); + expect(el.querySelector('[role="alert"]')?.textContent).toContain("Disk full"); +}); + +test("rule history shows imported versions in chronological order", () => { + const data = createSeedDataset("2026-01"); + const post = data.posts[0]!; + post.rules = [ + { from: "2026-07", rule: { kind: "percentOfIncome", percent: 15 } }, + { from: "2026-01", rule: { kind: "percentOfIncome", percent: 10 } }, + ]; + const el = mount(); + const months = Array.from(el.querySelectorAll("tbody tr td:first-child"), (cell) => cell.textContent); + expect(months).toEqual(["2026-01", "2026-07"]); +}); diff --git a/src/ui/components/RuleHistory.tsx b/src/ui/components/RuleHistory.tsx index 285e002..9d5feaf 100755 --- a/src/ui/components/RuleHistory.tsx +++ b/src/ui/components/RuleHistory.tsx @@ -27,12 +27,13 @@ const MONTH_ID = /^\d{4}-(0[1-9]|1[0-2])$/; * so rather than letting it look like an append. */ export function RuleHistory({ post }: Props) { - const { mutate } = useMutate(); + const { mutate, error, clearError } = useMutate(); const [from, setFrom] = useState(currentMonth); const [kind, setKind] = useState("percentOfIncome"); // Held as text, not a number, so the field can be cleared and can hold a // half-typed "12." mid-entry. Parsed once, on submit. const [value, setValue] = useState(""); + const versions = [...post.rules].sort((a, b) => a.from.localeCompare(b.from)); const replaces = post.rules.some((v) => v.from === from); const monthOk = MONTH_ID.test(from); @@ -44,7 +45,8 @@ export function RuleHistory({ post }: Props) { kind === "fixed" ? { kind: "fixed", amount: { amount, currency: post.currency } } : { kind: "percentOfIncome", percent: amount }; - mutate((draft) => setRuleFrom(draft, post.id, from, rule)); + clearError(); + void mutate((draft) => setRuleFrom(draft, post.id, from, rule)); } return ( @@ -64,7 +66,7 @@ export function RuleHistory({ post }: Props) { - {post.rules.map((version) => ( + {versions.map((version) => ( {version.from} @@ -76,9 +78,10 @@ export function RuleHistory({ post }: Props) { @@ -137,6 +140,7 @@ export function RuleHistory({ post }: Props) { )} + {error &&

Could not save: {error}

} ); } diff --git a/src/ui/routes/MonthRoute.tsx b/src/ui/routes/MonthRoute.tsx index d62d41b..16a7e4d 100644 --- a/src/ui/routes/MonthRoute.tsx +++ b/src/ui/routes/MonthRoute.tsx @@ -300,7 +300,7 @@ function MonthStep({ to, label, back = false }: { to: string; label: string; bac * same `setRuleFrom` action the Settings history editor uses, so the two * surfaces cannot drift apart. */ -function RuleFromMonth({ +export function RuleFromMonth({ post, monthId, initial, @@ -311,7 +311,7 @@ function RuleFromMonth({ initial: Rule | undefined; onDone: () => void; }) { - const { mutate } = useMutate(); + const { mutate, error, clearError } = useMutate(); // Pre-filled with what is currently in effect, so nudging 10% to 15% is an // edit rather than re-entry. Empty when the post has no rule yet. Held as // text so the field can be cleared mid-edit; parsed once, on apply. @@ -321,6 +321,9 @@ function RuleFromMonth({ ? "" : String(initial.kind === "fixed" ? initial.amount.amount : initial.percent), ); + const [currency] = useState(() => + initial?.kind === "fixed" ? initial.amount.currency : post.currency, + ); const amount = Number(value); const valueOk = value.trim() !== "" && Number.isFinite(amount); @@ -328,10 +331,12 @@ function RuleFromMonth({ function apply() { const rule: Rule = kind === "fixed" - ? { kind: "fixed", amount: { amount, currency: post.currency } } + ? { kind: "fixed", amount: { amount, currency } } : { kind: "percentOfIncome", percent: amount }; - mutate((draft) => setRuleFrom(draft, post.id, monthId, rule)); - onDone(); + clearError(); + void mutate((draft) => setRuleFrom(draft, post.id, monthId, rule)).then((saved) => { + if (saved) onDone(); + }); } return ( @@ -355,7 +360,7 @@ function RuleFromMonth({ className="font-money h-8 w-24" type="number" step="0.01" - aria-label={kind === "fixed" ? `Amount in ${post.currency}` : "Percent of income"} + aria-label={kind === "fixed" ? `Amount in ${currency}` : "Percent of income"} value={value} onChange={(event) => { const next = event.target.value; @@ -368,6 +373,7 @@ function RuleFromMonth({ + {error &&

Could not save: {error}

} ); }