From 064cea25fd27af13e5871588bc6d654d5ec89d6e Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Tue, 13 Oct 2026 13:24:41 +0200 Subject: [PATCH] fix(goal): never drop an unreadable goal by accident, and let the count be retyped Review found three ordinary actions that deleted a goal this build could not read: "It repeats daily", pressing an already-pressed Off, and "not daily" without saying so. Only "not daily" clears a goal now, and its warning keys off what is stored, not what parses. The count field was bound straight to the goal, so emptying it snapped the old number back and typing appended to it. It now keeps a draft. Co-Authored-By: Claude Opus 5.5 --- docs/DATA-MODEL.md | 6 +- src/client/App.tsx | 1 + src/client/components/EventDetail.tsx | 13 ++- src/client/components/GoalControl.tsx | 112 ++++++++++++++++++-------- src/client/state/useProgress.ts | 24 ++++-- test/custom-ui.test.tsx | 17 ++++ test/progress.test.ts | 15 +++- test/views.test.tsx | 33 +++++++- 8 files changed, 177 insertions(+), 44 deletions(-) diff --git a/docs/DATA-MODEL.md b/docs/DATA-MODEL.md index 7a8ff26..8a6b079 100644 --- a/docs/DATA-MODEL.md +++ b/docs/DATA-MODEL.md @@ -291,8 +291,10 @@ out is a record marked only with that field, silently dropped on the next write. removes an unreadable goal from the record — the likeliest one is a newer shape (weekly tasks add `measure: "amount"`) met by an older cached bundle, and dropping it on load would delete it on the next write. A goal makes the event a daily regardless of `daily` and detection; setting one clears a -stored `daily: false`, and any `daily` answer clears the goal, so the two never contradict in -storage. +stored `daily: false`, and answering "this isn't a daily event" clears the goal, so the two never +contradict in storage. No other control removes a goal — not "it repeats daily", and not pressing +*Off* when no goal is readable — because either could be standing on one this build cannot see. The +"not daily" control says it removes the goal whenever *anything* is stored there, readable or not. **`effort` is load-bearing, not decorative.** Combined with the time remaining it answers "can I still finish this?" — the same two days is comfortable for a `quick` event and hopeless for a diff --git a/src/client/App.tsx b/src/client/App.tsx index 31ed848..2f8084a 100644 --- a/src/client/App.tsx +++ b/src/client/App.tsx @@ -761,6 +761,7 @@ export function App() { detectedDaily={prefs.detectDaily && isDaily(openRow.event)} dailyDays={daily.daysFor(openRow.event.id)} goal={goalOf(openRow.event.id)} + hasStoredGoal={prog.progress[openRow.event.id]?.goal !== undefined} onDaily={prog.setDaily} onGoal={prog.setGoal} onToggleDay={daily.toggleDay} diff --git a/src/client/components/EventDetail.tsx b/src/client/components/EventDetail.tsx index 020b6e3..07fc805 100644 --- a/src/client/components/EventDetail.tsx +++ b/src/client/components/EventDetail.tsx @@ -38,6 +38,7 @@ export function EventDetail({ detectedDaily, dailyDays, goal, + hasStoredGoal, onDaily, onGoal, onToggleDay, @@ -64,7 +65,13 @@ export function EventDetail({ dailyDays: string[]; /** The reader's check-in goal, already read through `readGoal`. */ goal: Goal | undefined; - onDaily: (id: string, daily: boolean | undefined) => void; + /** + * Whether anything is stored under the goal at all, readable or not. A goal + * of a newer shape reads as none, and "not daily" would still remove it, so + * the warning keys off this rather than off `goal`. + */ + hasStoredGoal: boolean; + onDaily: (id: string, daily: boolean | undefined, clearsGoal?: boolean) => void; onGoal: (id: string, goal: Goal | undefined) => void; onToggleDay: (id: string, day: string) => void; onIgnore: (id: string) => void; @@ -246,10 +253,10 @@ export function EventDetail({ )} diff --git a/src/client/components/GoalControl.tsx b/src/client/components/GoalControl.tsx index 0552e6f..4cc16e1 100644 --- a/src/client/components/GoalControl.tsx +++ b/src/client/components/GoalControl.tsx @@ -1,3 +1,4 @@ +import { useEffect, useState } from "react"; import { clampRequired, type Goal } from "../../shared/goal.ts"; const WEEKDAYS = ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"]; @@ -8,6 +9,44 @@ const MODES = [ { id: "event", label: "Across the event" }, ] as const; +type Mode = (typeof MODES)[number]["id"]; + +/** + * The goal a mode press asks for: a Goal, undefined to clear it, or null for + * "nothing to do". Pressing the mode already shown is null — including Off + * when no goal is *readable*, which may still be one stored by a newer + * version, and a press that looks like a no-op must not delete it. + */ +export function chooseGoal( + id: Mode, + goal: Goal | undefined, + defaultWeekStartsOn: number, + maxDays: number | null, +): Goal | undefined | null { + if (id === (goal?.period ?? "off")) return null; + if (id === "off") return undefined; + const required = goal?.required ?? 5; + if (id === "week") { + return { + period: "week", + measure: "days", + required: clampRequired("week", required, null), + weekStartsOn: goal?.period === "week" ? goal.weekStartsOn : defaultWeekStartsOn, + }; + } + return { period: "event", measure: "days", required: clampRequired("event", required, maxDays) }; +} + +/** A typed count, clamped; null while the field is empty or unreadable — mid-edit, not an answer. */ +export function parseDraft( + raw: string, + period: Goal["period"], + maxDays: number | null, +): number | null { + const n = Number.parseInt(raw, 10); + return Number.isFinite(n) ? clampRequired(period, n, maxDays) : null; +} + /** * How many check-ins the event actually wants. * @@ -32,30 +71,10 @@ export function GoalControl({ onChange: (goal: Goal | undefined) => void; }) { const mode = goal?.period ?? "off"; - const required = goal?.required ?? 5; - const choose = (id: (typeof MODES)[number]["id"]) => { - if (id === "off") return onChange(undefined); - if (id === "week") { - return onChange({ - period: "week", - measure: "days", - required: clampRequired("week", required, null), - weekStartsOn: goal?.period === "week" ? goal.weekStartsOn : defaultWeekStartsOn, - }); - } - onChange({ - period: "event", - measure: "days", - required: clampRequired("event", required, maxDays), - }); - }; - - const setRequired = (raw: string) => { - if (goal === undefined) return; - // An emptied field is mid-edit, not an answer; keep the goal as it was. - if (raw.trim() === "") return; - onChange({ ...goal, required: clampRequired(goal.period, Number.parseInt(raw, 10), maxDays) }); + const choose = (id: Mode) => { + const next = chooseGoal(id, goal, defaultWeekStartsOn, maxDays); + if (next !== null) onChange(next); }; return ( @@ -84,15 +103,7 @@ export function GoalControl({ {goal !== undefined && (
- setRequired(e.target.value)} - className="tnum w-14 rounded-lg border border-hairline bg-ground px-2 py-1 text-sm text-ink focus:border-faint focus:outline-none" - /> + {goal.period === "week" ? "days each week, weeks start" : "days across the event"} @@ -115,3 +126,40 @@ export function GoalControl({
); } + +/** + * The count, with its own draft text. Bound straight to `goal.required`, the + * field could never be emptied: an empty value changes no goal, so React put + * the old number straight back, and typing after a backspace appended to it. + * The draft holds what is typed; a readable value commits, and leaving the + * field shows the goal's count again. + */ +function CountField({ + goal, + maxDays, + onChange, +}: { + goal: Goal; + maxDays: number | null; + onChange: (goal: Goal) => void; +}) { + const [draft, setDraft] = useState(String(goal.required)); + useEffect(() => setDraft(String(goal.required)), [goal.required]); + + return ( + { + setDraft(e.target.value); + const n = parseDraft(e.target.value, goal.period, maxDays); + if (n !== null && n !== goal.required) onChange({ ...goal, required: n }); + }} + onBlur={() => setDraft(String(goal.required))} + className="tnum w-14 rounded-lg border border-hairline bg-ground px-2 py-1 text-sm text-ink focus:border-faint focus:outline-none" + /> + ); +} diff --git a/src/client/state/useProgress.ts b/src/client/state/useProgress.ts index d36f51d..e0a5c86 100644 --- a/src/client/state/useProgress.ts +++ b/src/client/state/useProgress.ts @@ -88,6 +88,19 @@ export function applyPatch( return { ...prev, [id]: merged }; } +/** + * A daily answer. Only "not daily" clears a goal: the two would contradict. + * "It repeats daily" leaves `goal` untouched, because the event it is offered + * on may hold a goal this build cannot read — one it shows as none — and an + * answer about dailiness is no licence to delete it. + */ +export function dailyPatch( + daily: boolean | undefined, + clearsGoal: boolean, +): Partial { + return clearsGoal ? { daily, goal: undefined } : { daily }; +} + /** * Setting a goal makes the event a daily; a stored `daily: false` would * contradict it, so that alone is cleared. Any other `daily` answer stays, so @@ -167,12 +180,13 @@ export function useProgress() { // the press after "done" cleared the status instead of undoing it. Status is // set outright now, by a control per state. - // "This isn't a daily event" must not leave a goal hidden in storage that - // contradicts it, so any daily answer clears the goal in the same write. The - // other direction — "it repeats daily" — only ever runs on an event with no - // goal, because a goal already makes it daily. + // `clearsGoal` is the caller saying "this isn't a daily event": that answer + // must not leave a contradicting goal hidden in storage. The stored override + // alone cannot say which way the reader answered — agreeing with detection + // stores undefined either way — so the caller says. const setDaily = useCallback( - (id: string, daily: boolean | undefined) => patch(id, { daily, goal: undefined }), + (id: string, daily: boolean | undefined, clearsGoal = false) => + patch(id, dailyPatch(daily, clearsGoal)), [patch], ); diff --git a/test/custom-ui.test.tsx b/test/custom-ui.test.tsx index 04d099d..7c2d6a6 100644 --- a/test/custom-ui.test.tsx +++ b/test/custom-ui.test.tsx @@ -784,6 +784,7 @@ describe("the derived-boundary note", () => { detectedDaily: false, dailyDays: [], goal: undefined, + hasStoredGoal: false, onDaily: noop, onGoal: noop, onToggleDay: noop, @@ -858,6 +859,7 @@ describe("the derived-boundary note", () => { status={status} daily goal={{ period: "event", measure: "days", required: 5 }} + hasStoredGoal dailyDays={["2026-08-20", "2026-08-21", "2026-08-22", "2026-08-23", "2026-08-24"]} row={{ event: PARSED, clock: clockFor(PARSED, detailProps.region, NOW) }} /> @@ -869,6 +871,21 @@ describe("the derived-boundary note", () => { expect(open).toContain("This isn't a daily event — removes the goal"); expect(sheet("done")).not.toContain("Mark event done"); }); + + test("'not daily' warns it removes a goal even when this build cannot read it", () => { + const html = renderToStaticMarkup( + metaFor(id, GAMES)}> + + , + ); + expect(html).toContain("removes the goal"); + }); }); describe("the cadence control", () => { diff --git a/test/progress.test.ts b/test/progress.test.ts index 4c88e82..c38fe92 100644 --- a/test/progress.test.ts +++ b/test/progress.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { buildExportData, parseImportData } from "../src/client/state/export.ts"; import { restorePrefsValue } from "../src/client/state/usePrefs.ts"; -import { applyPatch, goalPatch, mergeProgress } from "../src/client/state/useProgress.ts"; +import { applyPatch, dailyPatch, goalPatch, mergeProgress } from "../src/client/state/useProgress.ts"; import type { ProgressMap } from "../src/client/state/useProgress.ts"; /** @@ -221,3 +221,16 @@ describe("goals on progress", () => { expect(parseImportData(old)!.progress).toEqual({ e: { status: "done", at: at("10") } }); }); }); + +describe("a goal this build cannot read is never cleared by accident", () => { + // Final review: "It repeats daily" wiped a newer-shaped goal it could not + // see. Only saying the event is *not* daily clears a goal. + test("answering 'it repeats daily' leaves the goal alone", () => { + expect(dailyPatch(true, false)).toStrictEqual({ daily: true }); + expect(dailyPatch(undefined, false)).toStrictEqual({ daily: undefined }); + }); + + test("answering 'not daily' clears it in the same write", () => { + expect(dailyPatch(false, true)).toStrictEqual({ daily: false, goal: undefined }); + }); +}); diff --git a/test/views.test.tsx b/test/views.test.tsx index 2c23b70..b11746b 100644 --- a/test/views.test.tsx +++ b/test/views.test.tsx @@ -17,7 +17,7 @@ import { } from "../src/client/components/Dailies.tsx"; import type { GoalStatus } from "../src/shared/goal.ts"; import { Welcome } from "../src/client/components/Welcome.tsx"; -import { GoalControl } from "../src/client/components/GoalControl.tsx"; +import { chooseGoal, GoalControl, parseDraft } from "../src/client/components/GoalControl.tsx"; import { DailyChecklist } from "../src/client/components/DailyChecklist.tsx"; import { timelineLanes } from "../src/client/state/lanes.ts"; import { GameMetaProvider } from "../src/client/state/gameMeta.tsx"; @@ -1288,3 +1288,34 @@ describe("DailyChecklist with a goal", () => { expect(html).not.toContain("missed"); }); }); + +describe("GoalControl decisions", () => { + const weekly = { period: "week", measure: "days", required: 5, weekStartsOn: 1 } as const; + + test("pressing Off when no goal is readable changes nothing", () => { + // An unreadable stored goal reads as none, so Off already shows pressed; + // pressing it must not delete what is stored. + expect(chooseGoal("off", undefined, 1, 84)).toBeNull(); + expect(chooseGoal("off", weekly, 1, 84)).toBeUndefined(); + }); + + test("pressing the mode already chosen changes nothing", () => { + expect(chooseGoal("week", weekly, 1, 84)).toBeNull(); + }); + + test("switching modes keeps the count, clamped", () => { + expect(chooseGoal("event", weekly, 1, 3)).toEqual({ + period: "event", measure: "days", required: 3, + }); + expect(chooseGoal("week", undefined, 4, 84)).toEqual({ + period: "week", measure: "days", required: 5, weekStartsOn: 4, + }); + }); + + test("an emptied or unreadable field is mid-edit, not an answer", () => { + expect(parseDraft("", "week", null)).toBeNull(); + expect(parseDraft("abc", "week", null)).toBeNull(); + expect(parseDraft("3", "week", null)).toBe(3); + expect(parseDraft("53", "week", null)).toBe(7); + }); +});