From 5a4c746a446c0050305e46378a8733ac2f4bde82 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Tue, 13 Oct 2026 13:54:29 +0200 Subject: [PATCH] fix(tasks): keep a task whole through import and edit, and fix two counts Review found that import and edit rebuilt a task from this build's parse. That stripped any field it didn't know, and import dropped a newer version's task entirely. Both now keep the record as stored and validate only to display it. An event whose goal is points can't be ticked, so it no longer counts as waiting today: it counts with its week. The month-day field reset itself to 1 when emptied; it now keeps a draft. Co-Authored-By: Claude Opus 5.5 --- docs/DATA-MODEL.md | 4 ++- src/client/components/Dailies.tsx | 12 ++++++--- src/client/components/TaskForm.tsx | 17 +++++++++--- src/client/state/useTasks.ts | 43 ++++++++++++++++++------------ test/custom-ui.test.tsx | 11 +++++++- test/tasks-store.test.ts | 27 ++++++++++++++++++- test/views.test.tsx | 24 +++++++++++++++++ 7 files changed, 110 insertions(+), 28 deletions(-) diff --git a/docs/DATA-MODEL.md b/docs/DATA-MODEL.md index 3e165c3..2f2f6d0 100644 --- a/docs/DATA-MODEL.md +++ b/docs/DATA-MODEL.md @@ -634,7 +634,9 @@ timestamps keeps the merge order-independent and idempotent either way, and remo `mergeProgress` is pure and `test/progress.test.ts` pins it. Check-in goals ride `progress`, so they need no export key of their own and merge with the record they belong to. `tasks` and `amounts` are additive export keys like `customGames`: a file without them is from a device that had none. Tasks merge by id with the later -`updatedAt` winning (`mergeTasks`), incoming records validated and current ones kept as stored; +`updatedAt` winning (`mergeTasks`); an incoming record is let in on its identity alone (a `mytask:` +id and an `updatedAt`) and stored exactly as it came, so a newer version's task — or a field this +build does not know — survives a trip through an older build, and an edit keeps such fields too; amounts merge per item **and per period** with the later `at` winning (`mergeAmounts`), so restoring an old backup cannot roll this week's number back, and a week only one side has survives. An export written before daily checklists existed simply has no `daily` key, which is not an error. Losing a user's marks to a bad import is unrecoverable, so the merge is diff --git a/src/client/components/Dailies.tsx b/src/client/components/Dailies.tsx index 99aa668..816e8ef 100644 --- a/src/client/components/Dailies.tsx +++ b/src/client/components/Dailies.tsx @@ -61,9 +61,11 @@ export interface DailyItem { goal?: GoalStatus | undefined; /** * How often it is due. Chores and events are daily and make up the header's - * "today" count; tasks are counted by their own period beside it. + * "today" count; tasks are counted by their own period beside it. An event + * whose goal is a number has no "today" — nothing ticks — so it counts with + * its week, or, over its whole run, in no tally at all. */ - cadence: "day" | "week" | "month"; + cadence: "day" | "week" | "month" | "event"; /** A task's period, for finding the one before it in Catch up. */ period?: Period | undefined; /** A task's unit, printed after its target. */ @@ -149,6 +151,7 @@ export function dailyGroups( for (const event of events) { if (event.game !== lane) continue; + const goal = goalFor(event.id); items.push({ key: event.id, game: lane, @@ -158,8 +161,8 @@ export function dailyGroups( today, resetsIn, notBefore: startOf(event), - goal: goalFor(event.id), - cadence: "day", + goal, + cadence: goal?.measure !== "amount" ? "day" : goal.scope === "week" ? "week" : "event", }); } @@ -221,6 +224,7 @@ export function dailyTally( const month = { total: 0, complete: 0 }; for (const i of items) { const met = i.goal?.met === true; + if (i.cadence === "event") continue; if (i.cadence !== "day") { const tally = i.cadence === "week" ? week : month; tally.total += 1; diff --git a/src/client/components/TaskForm.tsx b/src/client/components/TaskForm.tsx index 3c3bb58..26270f6 100644 --- a/src/client/components/TaskForm.tsx +++ b/src/client/components/TaskForm.tsx @@ -13,23 +13,32 @@ export interface TaskFields { title: string; unit: "week" | "month"; weekday: number; - monthDay: number; + /** As typed, so the field can be emptied and retyped; read through `parseMonthDay`. */ + monthDay: string | number; kind: "days" | "amount"; required: string; target: string; amountUnit: string; } +/** A typed day of the month, clamped to 1–31; null while empty or unreadable. */ +export function parseMonthDay(raw: string): number | null { + const n = Number.parseInt(raw, 10); + return Number.isFinite(n) ? Math.min(31, Math.max(1, n)) : null; +} + /** * The task these fields describe, or null while they do not describe one the * store would accept. Checked against `TaskSchema` itself, so the form can * never offer Save on something the store would then refuse. */ export function taskDraftFrom(f: TaskFields): TaskDraft | null { + const monthDay = parseMonthDay(String(f.monthDay)); + if (f.unit === "month" && monthDay === null) return null; const period = f.unit === "week" ? ({ unit: "week", startsOn: f.weekday } as const) - : ({ unit: "month", startsOn: f.monthDay } as const); + : ({ unit: "month", startsOn: monthDay ?? 1 } as const); const unit = f.amountUnit.trim(); const target = parseAmount(f.target); const measure = @@ -78,7 +87,7 @@ export function TaskForm({ title: initial?.title ?? "", unit: initial?.period.unit ?? "week", weekday: initial?.period.unit === "week" ? initial.period.startsOn : 1, - monthDay: initial?.period.unit === "month" ? initial.period.startsOn : 1, + monthDay: String(initial?.period.unit === "month" ? initial.period.startsOn : 1), kind: initial?.measure.kind ?? "days", required: initial?.measure.kind === "days" ? String(initial.measure.required) : "1", target: initial?.measure.kind === "amount" ? String(initial.measure.target) : "", @@ -156,7 +165,7 @@ export function TaskForm({ min={1} max={31} value={f.monthDay} - onChange={(e) => set({ monthDay: Number.parseInt(e.target.value, 10) || 1 })} + onChange={(e) => set({ monthDay: e.target.value })} className={`tnum w-14 ${field}`} /> diff --git a/src/client/state/useTasks.ts b/src/client/state/useTasks.ts index 5339b82..166ba52 100644 --- a/src/client/state/useTasks.ts +++ b/src/client/state/useTasks.ts @@ -1,6 +1,6 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import type { Measure, Task } from "../../shared/task.ts"; -import { mintTaskId, TaskSchema } from "../../shared/task.ts"; +import { mintTaskId, TaskId, TaskSchema } from "../../shared/task.ts"; import type { Period } from "../../shared/period.ts"; import { KEYS, readJson, writeJson } from "./storage.ts"; @@ -39,24 +39,39 @@ const updatedAtOf = (v: unknown): string => { /** * Import: union by id, the later `updatedAt` winning, nothing removed. * - * Incoming records are validated — a file is untrusted input — and one that - * does not parse is skipped. Records already on the device are kept as they - * are, readable or not. + * A record is let in on its identity alone — a `mytask:` id and a timestamp — + * and stored **exactly as it came**, not as this build parses it. A backup is + * where a newer version's task (or a field this build does not know) has to + * survive a trip through an older one; parsing on the way in would strip the + * field or drop the task, and re-importing could never bring either back. + * Whether it can be *shown* is `visibleTasks`' question. */ export function mergeTasks(current: StoredTasks, incoming: unknown): StoredTasks { if (typeof incoming !== "object" || incoming === null) return current; const next: StoredTasks = { ...current }; for (const value of Object.values(incoming as Record)) { - const parsed = TaskSchema.safeParse(value); - if (!parsed.success) continue; - const id = parsed.data.id; - if (next[id] === undefined || parsed.data.updatedAt > updatedAtOf(next[id])) { - next[id] = parsed.data; + if (typeof value !== "object" || value === null) continue; + const id = (value as { id?: unknown }).id; + if (!TaskId.safeParse(id).success || updatedAtOf(value) === "") continue; + const key = id as string; + if (next[key] === undefined || updatedAtOf(value) > updatedAtOf(next[key])) { + next[key] = value; } } return next; } +/** + * A stored task with the reader's edit applied, or null if it is not one this + * build can edit. Built on the stored record, not on its parse, so a field a + * newer version added rides through the edit untouched. + */ +export function editedTask(stored: unknown, draft: TaskDraft, at: string): unknown { + if (!TaskSchema.safeParse(stored).success) return null; + const next = { ...(stored as Record), ...draft, updatedAt: at }; + return TaskSchema.safeParse(next).success ? next : null; +} + /** * The reader's weekly and monthly tasks (PRD F17). * @@ -86,14 +101,8 @@ export function useTasks() { const editTask = useCallback((id: string, draft: TaskDraft) => { setRaw((prev) => { - const existing = TaskSchema.safeParse(prev[id]); - if (!existing.success) return prev; - const next = TaskSchema.parse({ - ...existing.data, - ...draft, - updatedAt: new Date().toISOString(), - }); - return { ...prev, [id]: next }; + const next = editedTask(prev[id], draft, new Date().toISOString()); + return next === null ? prev : { ...prev, [id]: next }; }); }, []); diff --git a/test/custom-ui.test.tsx b/test/custom-ui.test.tsx index e00be35..2d61ec3 100644 --- a/test/custom-ui.test.tsx +++ b/test/custom-ui.test.tsx @@ -10,7 +10,7 @@ import { strandedNotice, } from "../src/client/components/CustomForms.tsx"; import { eventCaption, YourOwn } from "../src/client/components/YourOwn.tsx"; -import { TaskForm, taskDraftFrom } from "../src/client/components/TaskForm.tsx"; +import { parseMonthDay, TaskForm, taskDraftFrom } from "../src/client/components/TaskForm.tsx"; import { EventRow } from "../src/client/components/EventRow.tsx"; import { EventDetail } from "../src/client/components/EventDetail.tsx"; import { AUTHOR, Colophon, REPO_URL, RESET_CLOCK_CREDIT } from "../src/client/components/Colophon.tsx"; @@ -1311,3 +1311,12 @@ describe("the reader's weekly and monthly tasks in settings", () => { }); }); }); + +describe("review fixes: the month-day field", () => { + test("an emptied field is mid-edit, not day 1", () => { + expect(parseMonthDay("")).toBeNull(); + expect(parseMonthDay("5")).toBe(5); + expect(parseMonthDay("40")).toBe(31); + expect(parseMonthDay("0")).toBe(1); + }); +}); diff --git a/test/tasks-store.test.ts b/test/tasks-store.test.ts index 4eb9545..d35a3af 100644 --- a/test/tasks-store.test.ts +++ b/test/tasks-store.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { mergeAmounts, readableAmounts } from "../src/client/state/useAmounts.ts"; -import { mergeTasks, visibleTasks } from "../src/client/state/useTasks.ts"; +import { editedTask, mergeTasks, visibleTasks } from "../src/client/state/useTasks.ts"; /** * The two stores weekly and monthly tasks add. Both are the only copy of what @@ -91,3 +91,28 @@ describe("amounts", () => { expect(merged).toEqual({}); }); }); + +describe("review fixes: a task's data outlives this build", () => { + test("merge keeps fields this build does not know", () => { + const withExtra = { ...task(A, "Weekly", "2026-10-05T00:00:00.000Z"), notes: "keep me" }; + expect(mergeTasks({}, { [A]: withExtra })[A]).toEqual(withExtra); + }); + + test("a newer version's task in a backup reaches a fresh device", () => { + expect(mergeTasks({}, { [FUTURE.id]: FUTURE })[FUTURE.id]).toEqual(FUTURE); + }); + + test("but junk without a task id or a timestamp still stays out", () => { + expect(mergeTasks({}, { x: { id: "myevent:abcdef1234", updatedAt: "2026" } })).toEqual({}); + expect(mergeTasks({}, { x: { id: "mytask:abcdef1234" } })).toEqual({}); + }); + + test("editing keeps fields this build does not know", () => { + const stored = { ...task(A, "Weekly", "2026-10-05T00:00:00.000Z"), notes: "keep me" }; + const next = editedTask(stored, { + game: "zzz", title: "Renamed", period: { unit: "week", startsOn: 1 }, + measure: { kind: "days", required: 1 }, + }, "2026-10-06T00:00:00.000Z"); + expect(next).toMatchObject({ title: "Renamed", notes: "keep me", updatedAt: "2026-10-06T00:00:00.000Z" }); + }); +}); diff --git a/test/views.test.tsx b/test/views.test.tsx index 8c803ce..221c4ef 100644 --- a/test/views.test.tsx +++ b/test/views.test.tsx @@ -1457,3 +1457,27 @@ describe("GoalControl with points", () => { expect(html).toContain("points each week, weeks start"); }); }); + +describe("review fixes: an event's points goal is not a daily", () => { + test("it is counted with the week, not as waiting today", () => { + const NOW = Date.parse("2026-10-15T12:00:00.000Z"); + const ev = { + id: "p1", game: "zzz", title: "Points event", type: "other", summary: null, + startsAt: "2026-10-01T00:00:00.000Z", startPrecision: "day", endsAt: null, + endPrecision: "unknown", regionScoped: false, regionEnds: null, + sourceUrl: "https://example.test", + } as never; + const week: GoalStatus = { + scope: "week", measure: "amount", periodStart: "2026-10-12", required: 2100, count: 100, + met: false, needed: 2000, open: 4, shortfall: false, + }; + const total: GoalStatus = { ...week, scope: "event", periodStart: "2026-10-01" }; + const tally = (goal: GoalStatus) => { + const [group] = dailyGroups(["zzz"], [ev], NOW, "europe", (id) => metaFor(id, {}), + (e) => Date.parse(e.startsAt), false, () => goal); + return dailyTally(group!.items, () => []); + }; + expect(tally(week)).toMatchObject({ total: 0, week: { total: 1, complete: 0 } }); + expect(tally(total)).toMatchObject({ total: 0, week: { total: 0 }, month: { total: 0 } }); + }); +});