From 33f7c77704c1cdc7b469d767072b77b20aa2cb9a Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 28 Aug 2026 04:57:48 +0200 Subject: [PATCH] Let a preset actually be saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the headline feature unreachable. `valid` still demanded an end date unconditionally, while every sibling rule had been gated on `datedWindow`. A fresh form starts with endKnown true and endDate empty, so picking weekly hid both the field and the checkbox and left Save disabled with nothing on screen a reader could do about it. Editing an existing event was fine, which is why no test caught it: every static render drives the form through `initial`, and cadenceOf never returns a preset for an event that has an end. So the three states that needed a click to reach are now pure functions that do not. endStated pins exactly that case. repeatOf takes the cadence and answers for all five, which also makes `until` surviving the preset path provable — mutating it away previously left the suite green. Two more from the same review. The unknown-end note keyed off the cadence, so custom with the repeat set to never promised a countdown that would never arrive; it keys off the rule now, which is what actually decides. And a delay whose start and end disagree about having a time of day can never land a whole unit from the anchor — unstatable, not merely unstated — so it says so rather than disabling Save in silence. "State it myself" was also a one-way door: nothing ever cleared it, so the measured reading could not be recovered without abandoning the form. Co-Authored-By: Claude Opus 5 (1M context) --- docs/PRD.md | 2 +- src/client/components/CustomForms.tsx | 122 ++++++++++++++++++++------ test/custom-ui.test.tsx | 85 +++++++++++++++++- 3 files changed, 180 insertions(+), 29 deletions(-) diff --git a/docs/PRD.md b/docs/PRD.md index b1ec5a4..837cc61 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -331,7 +331,7 @@ decides which dates are even questions: |---|---|---| | one-off | start, end, or "I don't know when it ends" | no rule | | daily / weekly / monthly | a start, and nothing else | no end, a one-unit cycle | -| custom | start, end, and how it repeats | whatever the reader states | +| custom | start, an end it still allows to be unknown, and how it repeats | whatever the reader states | **A preset carries no window.** Choosing weekly says the week *is* the window — each occurrence runs until the next opens — so there is no end date to type and diff --git a/src/client/components/CustomForms.tsx b/src/client/components/CustomForms.tsx index ef89a2d..eb239da 100644 --- a/src/client/components/CustomForms.tsx +++ b/src/client/components/CustomForms.tsx @@ -184,20 +184,64 @@ function openingControls( } /** - * The rule the three-way control currently describes. + * Whether the end-date question has been answered. * - * Pulled out of the component because it is the one place the three answers - * become the single `{unit, interval}` the schema stores, and that translation - * is worth reading in one piece rather than spread through the render. + * A cadence that renders no end field is asking nothing, so there is nothing + * to withhold. Missing that gate is not a cosmetic slip: a fresh form starts + * with `endKnown` true and `endDate` empty, so a preset — which hides both the + * field and the "I don't know" checkbox — would leave Save disabled with + * nothing on screen a reader could do about it. * - * A delay is expressed by where it lands: the next opening is the wait added - * to the close, and the interval is whatever spans the anchor to there. That - * is why a delay can never produce a rule that comes round before it ends — - * the next opening is at or after the close by construction. Only a - * hand-stated cadence can, which is why `comesRoundEarly` still guards the - * form. + * Exported because that state needs a click to reach, and nothing in this + * project can click. Every static render drives the form through `initial`, + * and `cadenceOf` never returns a preset for an event that has an end, so the + * broken case was invisible to the whole suite. */ -function repeatOf(input: { +export function endStated( + datedWindow: boolean, + endKnown: boolean, + endDate: string, +): boolean { + return !datedWindow || !endKnown || endDate !== ""; +} + +/** + * What an unstated end means, given whether anything repeats. + * + * Keyed off the rule rather than off the cadence. `custom` with the repeat set + * to never is a real state and it has no interval to bound anything, so + * choosing the sentence by cadence promised a countdown that would not arrive. + */ +export function unknownEndNote(repeat: Repeat | null): string { + if (repeat === null) { + return "It'll show with no countdown and no daily checklist, the same as an event whose source hasn't announced an end."; + } + return "Each one runs until the next one opens, so it still counts down."; +} + +/** + * The rule the form's controls currently describe, across all five cadences. + * + * The one place five answers become the single `{unit, interval}` the schema + * stores, which is worth reading in one piece rather than spread through the + * render — and exported so that `until` surviving every one of those paths is + * provable without a submit nothing in this suite can click. + * + * A preset is one unit with no window. A delay is expressed by where it lands: + * the next opening is the wait added to the close, and the interval is + * whatever spans the anchor to there. That is why a delay can never produce a + * rule that comes round before it ends — the next opening is at or after the + * close by construction. Only a hand-stated cycle length can, which is why + * `comesRoundEarly` still guards the form. + * + * A delay returns null when no whole unit spans the anchor to that opening, + * which happens whenever the start and the end disagree about having a + * time-of-day: nothing steps from 10:00 to a midnight. The schema anchors + * every occurrence to `startsAt`, so that rule is not merely unstated but + * unstatable, and the form says so rather than disabling Save in silence. + */ +export function repeatOf(input: { + cadence: Cadence; mode: RepeatMode; measuring: boolean; measured: Repeat | null; @@ -207,9 +251,12 @@ function repeatOf(input: { amount: number; until: string | null; }): Repeat | null { - const { mode, measuring, measured, startMs, contiguousMs, unit, amount, until } = + const { cadence, mode, measuring, measured, startMs, contiguousMs, unit, amount, until } = input; + if (cadence === "one-off") return null; + if (cadence !== "custom") return repeatFrom(PRESET_UNIT[cadence], 1, until); + if (mode === "never") return null; if (mode === "forever") { return measuring && measured !== null @@ -449,11 +496,8 @@ export function EventForm({ // occurrence runs until the next opens. const delayNeedsEnd = repeatMode === "delay" && endMs === null; - const repeat = preset - ? repeatFrom(PRESET_UNIT[cadence], 1, existingUntil) - : cadence === "one-off" - ? null - : repeatOf({ + const repeat = repeatOf({ + cadence, mode: repeatMode, measuring, measured, @@ -487,7 +531,7 @@ export function EventForm({ startsAt !== null && !backwards && !endMissing && - (!endKnown || endDate !== "") && + endStated(datedWindow, endKnown, endDate) && !earlyReturn && !repeatIncomplete; @@ -730,6 +774,24 @@ export function EventForm({ )} + {/* The way back. Taking the cadence over is meant to be a decision the + reader can undo, and without this it was a one-way door: nothing + else ever cleared `ownCadence`, so the measured reading could not + be recovered without abandoning the form. Only offered when there + is something to go back to. */} + {repeatMode === "forever" && ownCadence && measured !== null && ( +

+ {" "} + · {cadenceLabel(measured)} +

+ )} + {repeatMode === "delay" && ( <>
@@ -766,6 +828,19 @@ export function EventForm({ after it ends {repeat !== null ? ` · ${cadenceLabel(repeat)}` : ""}

+ {/* Every occurrence is anchored to the start, so the wait has to + land a whole number of units from it. It never can when one + boundary has a time of day and the other does not — nothing + steps from 10:00 to a midnight — and that is unstatable rather + than merely unstated, so it says so instead of leaving Save + disabled with no reason given. */} + {repeat === null && !delayNeedsEnd && ( +

+ Give the start and the end both a time, or neither — a wait has + to land a whole number of days from the start, and these don't + line up. +

+ )} )} @@ -788,16 +863,9 @@ export function EventForm({ /> - {datedWindow && !endKnown && cadence === "one-off" && ( + {datedWindow && !endKnown && (

- It'll show with no countdown and no daily checklist, the same as an - event whose source hasn't announced an end. -

- )} - {datedWindow && !endKnown && cadence === "custom" && ( - /* Not a degraded answer here — the interval bounds it. */ -

- Each one runs until the next one opens, so it still counts down. + {unknownEndNote(repeat)}

)} {backwards && ( diff --git a/test/custom-ui.test.tsx b/test/custom-ui.test.tsx index 088b147..cc1d7ff 100644 --- a/test/custom-ui.test.tsx +++ b/test/custom-ui.test.tsx @@ -2,6 +2,9 @@ import { describe, expect, test } from "bun:test"; import { renderToStaticMarkup } from "react-dom/server"; import { cadenceLabel, + endStated, + repeatOf, + unknownEndNote, EventForm, repeatFrom, strandedNotice, @@ -454,7 +457,7 @@ describe("stating a repeat", () => { expect(html).toContain("Cycle length"); }); - test("a delay needs an end date to be measured from", () => { + test("with no end date, the delay option is closed off", () => { const html = renderToStaticMarkup( { expect(html).not.toContain("no countdown"); }); }); + +describe("endStated", () => { + // Extracted because the bug it now pins was unreachable from any test in + // this project: every static render drives the form through `initial`, and + // `cadenceOf` never returns a preset for an event that has an end. The + // broken state — a *fresh* form switched to a preset — needs a click. + test("a cadence with no end field has nothing to answer", () => { + // The Critical case. A fresh form has endKnown true and endDate empty; + // picking weekly hides both the field and the checkbox, so requiring an + // end here disables Save with nothing on screen that could satisfy it. + expect(endStated(false, true, "")).toBe(true); + expect(endStated(false, false, "")).toBe(true); + }); + + test("a cadence that asks still has to be answered", () => { + expect(endStated(true, true, "")).toBe(false); + expect(endStated(true, true, "2026-09-08")).toBe(true); + expect(endStated(true, false, "")).toBe(true); + }); +}); + +describe("unknownEndNote", () => { + // Keyed off the rule, not the cadence: `custom` with Repeat set to never is + // a real state, and it has no interval to bound anything. + test("no rule means no countdown, whatever the cadence", () => { + expect(unknownEndNote(null)).toContain("no countdown"); + }); + + test("a rule bounds it, so it still counts down", () => { + const note = unknownEndNote({ unit: "weeks", interval: 1, until: null }); + expect(note).toContain("until the next one opens"); + expect(note).not.toContain("no countdown"); + }); +}); + +describe("repeatOf", () => { + const base = { + mode: "forever" as const, + measuring: false, + measured: null, + startMs: Date.parse("2026-09-01T00:00:00.000Z"), + contiguousMs: Date.parse("2026-09-08T00:00:00.000Z"), + unit: "weeks" as const, + amount: 1, + until: null as string | null, + }; + + test("a one-off states no rule", () => { + expect(repeatOf({ ...base, cadence: "one-off" })).toBe(null); + }); + + test("each preset is its own unit, once", () => { + expect(repeatOf({ ...base, cadence: "daily" })).toEqual({ unit: "days", interval: 1, until: null }); + expect(repeatOf({ ...base, cadence: "weekly" })).toEqual({ unit: "weeks", interval: 1, until: null }); + expect(repeatOf({ ...base, cadence: "monthly" })).toEqual({ unit: "months", interval: 1, until: null }); + }); + + test("a preset carries an existing `until` through", () => { + // The form has no control for `until`, so the only way it survives an + // edit is by being threaded through every path that builds a rule — and + // the preset path is the newest of them. + const until = "2027-01-01T00:00:00.000Z"; + expect(repeatOf({ ...base, cadence: "weekly", until })?.until).toBe(until); + expect(repeatOf({ ...base, cadence: "custom", until })?.until).toBe(until); + }); + + test("a delay that cannot land on a whole unit states no rule", () => { + // Start at 10:00, end at midnight: no whole number of any unit steps from + // one to the other, so there is nothing the schema could store. + expect( + repeatOf({ + ...base, + cadence: "custom", + mode: "delay", + startMs: Date.parse("2026-09-01T10:00:00.000Z"), + contiguousMs: Date.parse("2026-09-08T00:00:00.000Z"), + }), + ).toBe(null); + }); +});