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 <noreply@anthropic.com>
This commit is contained in:
1 parent
d5ffad38c5
commit
064cea25fd
8 files changed
+177
-44
No files matched your search
+4
-2
@@ -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
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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({
|
||||
)}
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => onDaily(event.id, dailyOverride(false, detectedDaily))}
|
||||
onClick={() => onDaily(event.id, dailyOverride(false, detectedDaily), true)}
|
||||
className="mt-2 block text-xs text-faint transition-colors hover:text-muted"
|
||||
>
|
||||
{goal === undefined
|
||||
{!hasStoredGoal
|
||||
? "This isn't a daily event"
|
||||
: "This isn't a daily event — removes the goal"}
|
||||
</button>
|
||||
|
||||
@@ -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 && (
|
||||
<div className="mt-2 flex flex-wrap items-center gap-2 text-xs text-muted">
|
||||
<input
|
||||
type="number"
|
||||
aria-label="Days needed"
|
||||
min={1}
|
||||
max={goal.period === "week" ? 7 : (maxDays ?? 366)}
|
||||
value={goal.required}
|
||||
onChange={(e) => 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"
|
||||
/>
|
||||
<CountField goal={goal} maxDays={maxDays} onChange={onChange} />
|
||||
<span>
|
||||
{goal.period === "week" ? "days each week, weeks start" : "days across the event"}
|
||||
</span>
|
||||
@@ -115,3 +126,40 @@ export function GoalControl({
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* 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 (
|
||||
<input
|
||||
type="number"
|
||||
aria-label="Days needed"
|
||||
min={1}
|
||||
max={goal.period === "week" ? 7 : (maxDays ?? 366)}
|
||||
value={draft}
|
||||
onChange={(e) => {
|
||||
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"
|
||||
/>
|
||||
);
|
||||
}
|
||||
@@ -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<Progress> {
|
||||
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],
|
||||
);
|
||||
|
||||
|
||||
@@ -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(
|
||||
<GameMetaProvider value={(id) => metaFor(id, GAMES)}>
|
||||
<EventDetail
|
||||
{...detailProps}
|
||||
daily
|
||||
goal={undefined}
|
||||
hasStoredGoal
|
||||
row={{ event: PARSED, clock: clockFor(PARSED, detailProps.region, NOW) }}
|
||||
/>
|
||||
</GameMetaProvider>,
|
||||
);
|
||||
expect(html).toContain("removes the goal");
|
||||
});
|
||||
});
|
||||
|
||||
describe("the cadence control", () => {
|
||||
|
||||
+14
-1
@@ -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 });
|
||||
});
|
||||
});
|
||||
+32
-1
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user