Let a preset actually be saved
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) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
4251717615
commit
33f7c77704
+1
-1
@@ -331,7 +331,7 @@ decides which dates are even questions:
|
|||||||
|---|---|---|
|
|---|---|---|
|
||||||
| one-off | start, end, or "I don't know when it ends" | no rule |
|
| 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 |
|
| 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 —
|
**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
|
each occurrence runs until the next opens — so there is no end date to type and
|
||||||
|
|||||||
@@ -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
|
* A cadence that renders no end field is asking nothing, so there is nothing
|
||||||
* become the single `{unit, interval}` the schema stores, and that translation
|
* to withhold. Missing that gate is not a cosmetic slip: a fresh form starts
|
||||||
* is worth reading in one piece rather than spread through the render.
|
* 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
|
* Exported because that state needs a click to reach, and nothing in this
|
||||||
* to the close, and the interval is whatever spans the anchor to there. That
|
* project can click. Every static render drives the form through `initial`,
|
||||||
* is why a delay can never produce a rule that comes round before it ends —
|
* and `cadenceOf` never returns a preset for an event that has an end, so the
|
||||||
* the next opening is at or after the close by construction. Only a
|
* broken case was invisible to the whole suite.
|
||||||
* hand-stated cadence can, which is why `comesRoundEarly` still guards the
|
|
||||||
* form.
|
|
||||||
*/
|
*/
|
||||||
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;
|
mode: RepeatMode;
|
||||||
measuring: boolean;
|
measuring: boolean;
|
||||||
measured: Repeat | null;
|
measured: Repeat | null;
|
||||||
@@ -207,9 +251,12 @@ function repeatOf(input: {
|
|||||||
amount: number;
|
amount: number;
|
||||||
until: string | null;
|
until: string | null;
|
||||||
}): Repeat | null {
|
}): Repeat | null {
|
||||||
const { mode, measuring, measured, startMs, contiguousMs, unit, amount, until } =
|
const { cadence, mode, measuring, measured, startMs, contiguousMs, unit, amount, until } =
|
||||||
input;
|
input;
|
||||||
|
|
||||||
|
if (cadence === "one-off") return null;
|
||||||
|
if (cadence !== "custom") return repeatFrom(PRESET_UNIT[cadence], 1, until);
|
||||||
|
|
||||||
if (mode === "never") return null;
|
if (mode === "never") return null;
|
||||||
if (mode === "forever") {
|
if (mode === "forever") {
|
||||||
return measuring && measured !== null
|
return measuring && measured !== null
|
||||||
@@ -449,11 +496,8 @@ export function EventForm({
|
|||||||
// occurrence runs until the next opens.
|
// occurrence runs until the next opens.
|
||||||
const delayNeedsEnd = repeatMode === "delay" && endMs === null;
|
const delayNeedsEnd = repeatMode === "delay" && endMs === null;
|
||||||
|
|
||||||
const repeat = preset
|
const repeat = repeatOf({
|
||||||
? repeatFrom(PRESET_UNIT[cadence], 1, existingUntil)
|
cadence,
|
||||||
: cadence === "one-off"
|
|
||||||
? null
|
|
||||||
: repeatOf({
|
|
||||||
mode: repeatMode,
|
mode: repeatMode,
|
||||||
measuring,
|
measuring,
|
||||||
measured,
|
measured,
|
||||||
@@ -487,7 +531,7 @@ export function EventForm({
|
|||||||
startsAt !== null &&
|
startsAt !== null &&
|
||||||
!backwards &&
|
!backwards &&
|
||||||
!endMissing &&
|
!endMissing &&
|
||||||
(!endKnown || endDate !== "") &&
|
endStated(datedWindow, endKnown, endDate) &&
|
||||||
!earlyReturn &&
|
!earlyReturn &&
|
||||||
!repeatIncomplete;
|
!repeatIncomplete;
|
||||||
|
|
||||||
@@ -730,6 +774,24 @@ export function EventForm({
|
|||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
|
{/* 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 && (
|
||||||
|
<p className="mt-1.5 text-xs leading-relaxed text-faint">
|
||||||
|
<button
|
||||||
|
type="button"
|
||||||
|
onClick={() => setOwnCadence(false)}
|
||||||
|
className="underline transition-colors hover:text-ink"
|
||||||
|
>
|
||||||
|
use my dates instead
|
||||||
|
</button>{" "}
|
||||||
|
· {cadenceLabel(measured)}
|
||||||
|
</p>
|
||||||
|
)}
|
||||||
|
|
||||||
{repeatMode === "delay" && (
|
{repeatMode === "delay" && (
|
||||||
<>
|
<>
|
||||||
<div className="mt-2 grid grid-cols-2 gap-2">
|
<div className="mt-2 grid grid-cols-2 gap-2">
|
||||||
@@ -766,6 +828,19 @@ export function EventForm({
|
|||||||
after it ends
|
after it ends
|
||||||
{repeat !== null ? ` · ${cadenceLabel(repeat)}` : ""}
|
{repeat !== null ? ` · ${cadenceLabel(repeat)}` : ""}
|
||||||
</p>
|
</p>
|
||||||
|
{/* 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 && (
|
||||||
|
<p className="mt-2 text-xs text-critical">
|
||||||
|
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.
|
||||||
|
</p>
|
||||||
|
)}
|
||||||
</>
|
</>
|
||||||
)}
|
)}
|
||||||
</>
|
</>
|
||||||
@@ -788,16 +863,9 @@ export function EventForm({
|
|||||||
/>
|
/>
|
||||||
</label>
|
</label>
|
||||||
|
|
||||||
{datedWindow && !endKnown && cadence === "one-off" && (
|
{datedWindow && !endKnown && (
|
||||||
<p className="mt-2 text-xs leading-relaxed text-faint">
|
<p className="mt-2 text-xs leading-relaxed text-faint">
|
||||||
It'll show with no countdown and no daily checklist, the same as an
|
{unknownEndNote(repeat)}
|
||||||
event whose source hasn't announced an end.
|
|
||||||
</p>
|
|
||||||
)}
|
|
||||||
{datedWindow && !endKnown && cadence === "custom" && (
|
|
||||||
/* Not a degraded answer here — the interval bounds it. */
|
|
||||||
<p className="mt-2 text-xs leading-relaxed text-faint">
|
|
||||||
Each one runs until the next one opens, so it still counts down.
|
|
||||||
</p>
|
</p>
|
||||||
)}
|
)}
|
||||||
{backwards && (
|
{backwards && (
|
||||||
|
|||||||
+84
-1
@@ -2,6 +2,9 @@ import { describe, expect, test } from "bun:test";
|
|||||||
import { renderToStaticMarkup } from "react-dom/server";
|
import { renderToStaticMarkup } from "react-dom/server";
|
||||||
import {
|
import {
|
||||||
cadenceLabel,
|
cadenceLabel,
|
||||||
|
endStated,
|
||||||
|
repeatOf,
|
||||||
|
unknownEndNote,
|
||||||
EventForm,
|
EventForm,
|
||||||
repeatFrom,
|
repeatFrom,
|
||||||
strandedNotice,
|
strandedNotice,
|
||||||
@@ -454,7 +457,7 @@ describe("stating a repeat", () => {
|
|||||||
expect(html).toContain("Cycle length");
|
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(
|
const html = renderToStaticMarkup(
|
||||||
<EventForm
|
<EventForm
|
||||||
lanes={["mygame:limbus-company"]}
|
lanes={["mygame:limbus-company"]}
|
||||||
@@ -800,3 +803,83 @@ describe("a preset says its piece exactly once", () => {
|
|||||||
expect(html).not.toContain("no countdown");
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user