From 32319feeea25138514ec3482c3c0f85751d41466 Mon Sep 17 00:00:00 2001 From: yiekheng Date: Sun, 10 May 2026 12:29:10 +0800 Subject: [PATCH] fix(reminders): edit paused/ended one-off; lock list order; collapse swipe after action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small bugs the user surfaced after the swipe rollout: 1. Editing a paused/ended one-off reminder threw "Time is in the past" ---------------------------------------------------------------------- The four edit-section pages (account / message / groups / when) all POST through `updateReminderAction`. The action's "scheduledAt must be in the future" check fires on every submit — including the three section pages that don't change the time and just pass the original `scheduledAt` straight through. So a user editing the message body of a reminder they paused yesterday saw their save rejected with "Time is in the past". New pure helper `validateUpdateScheduledAt` in lib/reminder-update.ts keeps the future-time check in place for active reminders that are actually changing the time, but allows past timestamps when: - the reminder is paused or ended (won't fire while in those states regardless of what the row says about scheduledAt), OR - the submitted timestamp matches the existing one within a second of rounding (the form is a passthrough). Tests: 10 cases in `lib/reminder-update.test.ts` covering active future, active past, paused passthrough, ended passthrough, paused with deliberate change, sub-second drift tolerance, exact-NOW edge, null existing scheduledAt, malformed ISO. Also (drive-by, related): `updateReminderAction` no longer force- sets `status: "active"` on save. Editing a paused reminder's message shouldn't silently un-pause it. The user uses Restart for that. 2. Reminder list reshuffled after Pause/Restart ---------------------------------------------------------------------- The list defaulted to `sort=scheduled_desc`, so clicking Restart on row N (which moves scheduledAt forward to the next occurrence) flipped the row to row 0. Felt like the wrong action ran. Fixed: - Page now hard-codes `sort = "created_desc"` (created_at never changes, so a row stays where it is). - Sort dropdown removed from `` since it has nothing to drive anymore. Account + Group filters and the search box stay. 3. Swipe shelf stayed open after the action ran ---------------------------------------------------------------------- `SwipeableRow` keeps its offset in component state. When a shelf button submits the form, the page revalidates and re-renders, but React keeps the same row instance (matched by `key={reminder.id}`), so the open offset stuck around. Now both row sites encode the "row state" into the key: - reminders: `key={\`${reminder.id}-${reminder.status}\`}` - activity: `key={\`${run.id}-${run.archivedAt ? "1" : "0"}\`}` Status flip → key change → React unmounts/remounts → offset back to 0 → shelf closed. Costs nothing (these rows are cheap). Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/web/src/actions/reminders.ts | 22 ++- apps/web/src/app/activity/page.tsx | 4 +- apps/web/src/app/reminders/page.tsx | 16 +- .../src/components/reminder-filter-bar.tsx | 28 +--- apps/web/src/lib/reminder-update.test.ts | 137 ++++++++++++++++++ apps/web/src/lib/reminder-update.ts | 45 ++++++ 6 files changed, 212 insertions(+), 40 deletions(-) create mode 100644 apps/web/src/lib/reminder-update.test.ts create mode 100644 apps/web/src/lib/reminder-update.ts diff --git a/apps/web/src/actions/reminders.ts b/apps/web/src/actions/reminders.ts index 6cc8351..21dd967 100644 --- a/apps/web/src/actions/reminders.ts +++ b/apps/web/src/actions/reminders.ts @@ -12,6 +12,7 @@ import { db } from "@/lib/db"; import { getSeededOperator } from "@/lib/operator"; import { checkRateLimit } from "@/lib/rate-limit"; import { pgNotifyBot } from "@/lib/notify"; +import { validateUpdateScheduledAt } from "@/lib/reminder-update"; async function rateLimit(key: string) { const h = await headers(); @@ -427,13 +428,15 @@ export async function updateReminderAction( } scheduledAt = firstFire; } else { - scheduledAt = DateTime.fromISO(scheduledAtIso, { zone: timezone }).toJSDate(); - if (Number.isNaN(scheduledAt.getTime())) { - return { ok: false, error: "Invalid date" }; - } - if (scheduledAt.getTime() <= Date.now()) { - return { ok: false, error: "Time is in the past" }; - } + const validated = validateUpdateScheduledAt({ + iso: scheduledAtIso, + timezone, + existingStatus: existing.status, + existingScheduledAt: existing.scheduledAt, + now: new Date(), + }); + if (!validated.ok) return { ok: false, error: validated.error }; + scheduledAt = validated.scheduledAt; } const groups = await db.query.whatsappGroups.findMany({ @@ -456,7 +459,10 @@ export async function updateReminderAction( scheduledAt, rrule: rrule ?? null, timezone, - status: "active", + // Preserve the lifecycle status. Editing fields shouldn't + // implicitly re-activate a paused or ended reminder — the + // user can use the explicit Restart action for that. + status: existing.status, updatedAt: new Date(), }) .where(eq(reminders.id, reminderId)); diff --git a/apps/web/src/app/activity/page.tsx b/apps/web/src/app/activity/page.tsx index 23b704c..947ca75 100644 --- a/apps/web/src/app/activity/page.tsx +++ b/apps/web/src/app/activity/page.tsx @@ -281,7 +281,9 @@ export default async function ActivityPage({ searchParams }: PageProps) { return ( diff --git a/apps/web/src/app/reminders/page.tsx b/apps/web/src/app/reminders/page.tsx index d66ca71..1ba683b 100644 --- a/apps/web/src/app/reminders/page.tsx +++ b/apps/web/src/app/reminders/page.tsx @@ -131,9 +131,12 @@ export default async function RemindersPage({ searchParams }: PageProps) { sp.filter === "active" || sp.filter === "ended" || sp.filter === "paused" ? sp.filter : "all"; - const sort: SortKey = (VALID_SORT_KEYS as string[]).includes(sp.sort ?? "") - ? (sp.sort as SortKey) - : "scheduled_desc"; + // Sort is now fixed to `created_desc`. Reordering on every status flip + // (Restart, Pause, Edit) was causing rows to jump around the list, + // which made the swipe gesture feel like the wrong thing happened. + // `created_at` never changes so the row stays put. + const sort: SortKey = "created_desc"; + void VALID_SORT_KEYS; // kept for future use; no longer read from URL const op = await getSeededOperator(); const tz = op.defaultTimezone ?? "UTC"; @@ -322,7 +325,12 @@ export default async function RemindersPage({ searchParams }: PageProps) { return ( = [ - { value: "scheduled_desc", label: "Newest first" }, - { value: "scheduled_asc", label: "Oldest first" }, - { value: "created_desc", label: "Recently created" }, - { value: "name_asc", label: "Name A→Z" }, -]; - /** * URL-driven filter row for /reminders. The page reads searchParams * server-side and re-renders with the filtered+sorted dataset; this @@ -46,7 +38,6 @@ export function ReminderFilterBar({ accounts, groups }: FilterBarProps) { q: searchParams.get("q") ?? "", accountId: searchParams.get("accountId") ?? "", groupId: searchParams.get("groupId") ?? "", - sort: (searchParams.get("sort") ?? "scheduled_desc") as SortKey, }; const [q, setQ] = useState(initial.q); @@ -116,7 +107,7 @@ export function ReminderFilterBar({ accounts, groups }: FilterBarProps) { )} -
+
-
- - -
{hasActiveFilter && ( diff --git a/apps/web/src/lib/reminder-update.test.ts b/apps/web/src/lib/reminder-update.test.ts new file mode 100644 index 0000000..fd817a7 --- /dev/null +++ b/apps/web/src/lib/reminder-update.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect } from "vitest"; +import { validateUpdateScheduledAt } from "./reminder-update"; + +const NOW = new Date("2026-05-13T10:00:00Z"); +const PAST = new Date("2026-05-01T09:00:00Z"); +const FUTURE = new Date("2026-05-20T09:00:00Z"); +const TZ = "Asia/Kuala_Lumpur"; + +const isoOf = (d: Date) => d.toISOString(); + +describe("validateUpdateScheduledAt", () => { + it("rejects malformed ISO outright", () => { + const r = validateUpdateScheduledAt({ + iso: "not-a-date", + timezone: TZ, + existingStatus: "active", + existingScheduledAt: PAST, + now: NOW, + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toBe("Invalid date"); + }); + + it("active reminder, future timestamp → ok", () => { + const r = validateUpdateScheduledAt({ + iso: isoOf(FUTURE), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: FUTURE, + now: NOW, + }); + expect(r.ok).toBe(true); + }); + + it("active reminder, past timestamp that DIFFERS from existing → rejected", () => { + const r = validateUpdateScheduledAt({ + iso: isoOf(PAST), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: new Date("2026-05-02T09:00:00Z"), + now: NOW, + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error).toBe("Time is in the past"); + }); + + it("paused one-off, past timestamp matching existing → ALLOWED (the bug fix)", () => { + // The user paused a fired reminder; now they're editing the message + // body via /reminders/[id]/edit/message, which submits the original + // (past) scheduledAt unchanged. Must not 400. + const r = validateUpdateScheduledAt({ + iso: isoOf(PAST), + timezone: TZ, + existingStatus: "paused", + existingScheduledAt: PAST, + now: NOW, + }); + expect(r.ok).toBe(true); + if (r.ok) expect(r.scheduledAt.getTime()).toBe(PAST.getTime()); + }); + + it("ended one-off, past timestamp matching existing → ALLOWED", () => { + const r = validateUpdateScheduledAt({ + iso: isoOf(PAST), + timezone: TZ, + existingStatus: "ended", + existingScheduledAt: PAST, + now: NOW, + }); + expect(r.ok).toBe(true); + }); + + it("paused reminder, past timestamp that's been changed → still allowed (paused doesn't fire)", () => { + // Paused reminders aren't on the firing path so a stale time isn't + // dangerous — even if the user submits a different past time, we + // accept it; it'll fire only if/when the user explicitly Restarts. + const r = validateUpdateScheduledAt({ + iso: isoOf(new Date("2026-04-01T09:00:00Z")), + timezone: TZ, + existingStatus: "paused", + existingScheduledAt: PAST, + now: NOW, + }); + expect(r.ok).toBe(true); + }); + + it("active reminder, past-but-equal-to-existing timestamp → allowed", () => { + // Edge case: the user opened the edit-message page on an active + // reminder that's about to fire; by the time they submit, NOW has + // ticked past existing.scheduledAt. Don't reject — it's the same + // moment the reminder already represents. + const existing = new Date("2026-05-13T09:59:00Z"); // 1 minute before NOW + const r = validateUpdateScheduledAt({ + iso: isoOf(existing), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: existing, + now: NOW, + }); + expect(r.ok).toBe(true); + }); + + it("active reminder, exact NOW → rejected (must be strictly future)", () => { + const r = validateUpdateScheduledAt({ + iso: isoOf(NOW), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: new Date("2026-05-19T09:00:00Z"), + now: NOW, + }); + expect(r.ok).toBe(false); + }); + + it("treats existingScheduledAt=null as 'no match' (active + past → reject)", () => { + const r = validateUpdateScheduledAt({ + iso: isoOf(PAST), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: null, + now: NOW, + }); + expect(r.ok).toBe(false); + }); + + it("sub-second drift (~750ms) still counts as 'same as existing'", () => { + const exact = new Date("2026-05-13T05:00:00.000Z"); + const drifted = new Date("2026-05-13T05:00:00.750Z"); + const r = validateUpdateScheduledAt({ + iso: isoOf(drifted), + timezone: TZ, + existingStatus: "active", + existingScheduledAt: exact, + now: NOW, + }); + expect(r.ok).toBe(true); + }); +}); diff --git a/apps/web/src/lib/reminder-update.ts b/apps/web/src/lib/reminder-update.ts new file mode 100644 index 0000000..57c6bb8 --- /dev/null +++ b/apps/web/src/lib/reminder-update.ts @@ -0,0 +1,45 @@ +import { DateTime } from "luxon"; + +/** + * Resolve and validate the scheduledAt for `updateReminderAction`. + * + * Why this exists: the action is invoked from four edit forms + * (account / message / groups / when). Three of those don't change + * the time — they pass the original `scheduledAt` straight through. + * For a paused/ended one-off whose original time is now in the past + * (the operator paused it after a fire and is editing the message), + * a strict "must be future" check rejects the edit even though the + * operator only wanted to fix a typo. So: + * + * - Reject malformed ISO outright. + * - Allow past timestamps when: + * * the reminder's existing status is `paused` or `ended` — + * it won't fire in those states regardless, AND/OR + * * the submitted timestamp matches the existing one (within + * a second of rounding), i.e. the form is passing it through + * unchanged. + * - Otherwise reject past timestamps so an active reminder can't + * be scheduled into a moment that's already gone. + * + * Pure function — takes `now` as input so tests can pin the clock. + */ +export function validateUpdateScheduledAt(args: { + iso: string; + timezone: string; + existingStatus: string; + existingScheduledAt: Date | null; + now: Date; +}): { ok: true; scheduledAt: Date } | { ok: false; error: string } { + const dt = DateTime.fromISO(args.iso, { zone: args.timezone }).toJSDate(); + if (Number.isNaN(dt.getTime())) { + return { ok: false, error: "Invalid date" }; + } + const isPaused = args.existingStatus === "paused" || args.existingStatus === "ended"; + const sameAsExisting = + args.existingScheduledAt !== null && + Math.abs(args.existingScheduledAt.getTime() - dt.getTime()) < 1000; + if (dt.getTime() <= args.now.getTime() && !isPaused && !sameAsExisting) { + return { ok: false, error: "Time is in the past" }; + } + return { ok: true, scheduledAt: dt }; +}