fix(rules): review and harden rule-history editing

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

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", () => {

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)}"`,
         );

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(
+    <RuleFromMonth post={post} monthId="2026-01" initial={initial} onDone={() => {}} />,
+  );
+  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(
+    <RuleFromMonth
+      post={data.posts[0]!}
+      monthId="2026-01"
+      initial={{ kind: "percentOfIncome", percent: 10 }}
+      onDone={() => { 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(<RuleHistory post={post} />);
+
+  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(<RuleHistory post={post} />);
+  const months = Array.from(el.querySelectorAll("tbody tr td:first-child"), (cell) => cell.textContent);
+  expect(months).toEqual(["2026-01", "2026-07"]);
+});

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<string>(currentMonth);
   const [kind, setKind] = useState<Rule["kind"]>("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<string>("");
+  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) {
             </tr>
           </thead>
           <tbody>
-            {post.rules.map((version) => (
+            {versions.map((version) => (
               <tr key={version.from} className="border-t">
                 <td className="py-1 font-money">{version.from}</td>
                 <td className="py-1">
@@ -76,9 +78,10 @@ export function RuleHistory({ post }: Props) {
                   <Button
                     size="sm"
                     variant="ghost"
-                    onClick={() =>
-                      mutate((draft) => removeRuleFrom(draft, post.id, version.from))
-                    }
+                    onClick={() => {
+                      clearError();
+                      void mutate((draft) => removeRuleFrom(draft, post.id, version.from));
+                    }}
                   >
                     remove
                   </Button>
@@ -137,6 +140,7 @@ export function RuleHistory({ post }: Props) {
           </span>
         )}
       </div>
+      {error && <p role="alert" className="text-sm text-destructive">Could not save: {error}</p>}
     </div>
   );
 }

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({
       <Button size="sm" variant="ghost" onClick={onDone}>
         Cancel
       </Button>
+      {error && <p role="alert" className="w-full text-sm text-destructive">Could not save: {error}</p>}
     </div>
   );
 }