From f38864088b3adc9da1f68708dd85fde26d53c197 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 19 Aug 2026 21:22:22 -0400 Subject: [PATCH] core: a completed recurring reminder advances instead of ending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `complete_reminder` cleared `remind_at` and said so in its own comment — "(Recurrence advancement is a later refinement.)". So Done on a daily reminder was quietly the last time it ever fired. Reminder notifications made that much easier to hit, because Done is now a button in the notification shade. **The server already had this.** `src/thoughtsync/notes/recurrence.py` has done it correctly all along, which means the web behaved one way and the desktop and Android the other, on the same note, in the same account. This is a port of that file rather than a fresh implementation, kept behaviourally identical rather than merely similar: the same reminder can be completed from a browser or a client, and a disagreement would move it depending on which one you happened to use. The seven new tests in `core/src/local/recur.rs` mirror the Python suite case for case, including the one that matters most in practice — 31 January plus a month is 28 February, and the step after that is 28 March rather than back to the 31st. That clamp is sticky, and it is now asserted on both sides so a future "fix" to either has to change both. Advancement is measured from the reminder's own time, never from now, which is what keeps a 09:00 daily reminder at 09:00 when it is dealt with at 09:47. A phone left in a drawer for a fortnight rolls forward to tomorrow rather than arriving at fourteen pending occurrences of the same thing. Also matched from the server, and a latent bug of its own: the non-recurring branch now clears `recurrence` as well as `remind_at`. Before, completing a note that carried a rule left the rule behind with no reminder attached — invisible in every UI, since they only render recurrence when there is a reminder to recur from, and waiting to surprise whoever next set a time on that note. Documented rather than hidden, and shared with the server: the arithmetic is in UTC and a note carries no timezone, so a daily reminder crossing a DST boundary keeps its UTC time and shifts by an hour locally. Fixing that means a zone per note, which is a wire-format change. Verified in the CI image before pushing: fmt, clippy --all-targets -D warnings, and the full suite — core 89 to 96, ffi 11 to 12. The new FFI test walks the path the notification's Done button actually takes. --- android/ffi/src/lib.rs | 65 +++++++++++++++ core/src/local/mod.rs | 1 + core/src/local/recur.rs | 171 ++++++++++++++++++++++++++++++++++++++++ core/src/local/store.rs | 36 ++++++++- 4 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 core/src/local/recur.rs diff --git a/android/ffi/src/lib.rs b/android/ffi/src/lib.rs index 5b43216..6a35f25 100644 --- a/android/ffi/src/lib.rs +++ b/android/ffi/src/lib.rs @@ -755,6 +755,71 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// The path the notification's Done button takes. + /// + /// Completing a RECURRING reminder must move it, not end it — this is the + /// behaviour the web has had all along and the clients did not, which made + /// "Done" on a daily reminder quietly the last time it ever fired. + #[test] + fn completing_a_recurring_reminder_moves_it_rather_than_ending_it() { + let dir = scratch_dir(); + let app = ThoughtSync::new(dir.clone()).expect("a fresh data dir should open"); + let note = app + .create_note(draft("Water the plants", "")) + .expect("create"); + + let armed = app + .update_note( + note.id.clone(), + vec![ + NoteEdit::RemindAt { + value: "2026-07-01T09:00:00.000Z".into(), + }, + NoteEdit::Recurrence { + value: "daily".into(), + }, + ], + ) + .expect("arm a daily reminder"); + assert_eq!(armed.recurrence.as_deref(), Some("daily")); + + let done = app.complete_reminder(note.id.clone()).expect("complete"); + let next = done + .remind_at + .expect("a daily reminder must still have a next occurrence"); + assert!( + next.as_str() > "2026-07-01T09:00:00.000Z", + "it must move FORWARD, got {next:?}" + ); + assert!( + next.ends_with("T09:00:00.000Z"), + "the time of day is what was asked for and must survive, got {next:?}" + ); + assert_eq!( + done.recurrence.as_deref(), + Some("daily"), + "the rule outlives the occurrence" + ); + + // A one-off clears BOTH fields, so an unrecognised rule cannot linger + // invisibly on a note with no reminder. + let once = app + .create_note(draft("Post the letter", "")) + .expect("create"); + app.update_note( + once.id.clone(), + vec![NoteEdit::RemindAt { + value: "2026-07-01T09:00:00.000Z".into(), + }], + ) + .expect("arm a one-off"); + let finished = app.complete_reminder(once.id.clone()).expect("complete"); + assert_eq!(finished.remind_at, None); + assert_eq!(finished.recurrence, None); + + std::fs::remove_dir_all(&dir).ok(); + } + /// A crude RFC3339 sanity check that doesn't pull a date crate into this /// crate's dev-dependencies to assert one field is well-formed. fn chrono_free_parse(raw: &str) -> usize { diff --git a/core/src/local/mod.rs b/core/src/local/mod.rs index d153a2c..4ba8e4c 100644 --- a/core/src/local/mod.rs +++ b/core/src/local/mod.rs @@ -6,6 +6,7 @@ pub mod derive; pub mod models; +pub mod recur; pub mod retention; pub mod schema; pub mod store; diff --git a/core/src/local/recur.rs b/core/src/local/recur.rs new file mode 100644 index 0000000..8d76bf9 --- /dev/null +++ b/core/src/local/recur.rs @@ -0,0 +1,171 @@ +//! Recurring-reminder math: where a reminder goes when it is marked done. +//! +//! A deliberate port of the server's `src/thoughtsync/notes/recurrence.py`, kept +//! behaviourally identical rather than merely similar. The same note can be +//! completed from the web (server code) or from the desktop and Android (this +//! code), and the two must land on the same instant — otherwise completing a +//! reminder on a phone and then syncing would silently move it relative to +//! completing it in a browser, and neither surface would look wrong on its own. +//! +//! Advancement is measured from the reminder's OWN time, never from now. That is +//! what keeps a 09:00 daily reminder at 09:00 after being dealt with at 09:47, +//! and a monthly one on the same day of the month. +//! +//! Known limitation, shared with the server: the arithmetic is in UTC, and a note +//! carries no timezone. So a daily reminder crossing a DST boundary keeps its UTC +//! time and shifts by an hour locally. Fixing that means storing a zone per note +//! and is a change to the wire format, not to this file. + +use chrono::{DateTime, Duration, Months, Utc}; + +/// The four intervals every surface offers. Anything else is not a recurrence. +pub const RECURRENCES: [&str; 4] = ["daily", "weekly", "monthly", "yearly"]; + +/// A recurrence we recognise, or nothing. +/// +/// Values reach the store from three clients and a sync payload, so "not a rule +/// we know" is an ordinary case rather than a corruption to shout about. +pub fn normalize(value: Option<&str>) -> Option<&str> { + value.filter(|v| RECURRENCES.contains(v)) +} + +/// One step forward. `None` for an unrecognised rule. +/// +/// Months and years clamp the day to the target month's length — 31 January plus +/// a month is 28 February, and the following step is 28 March rather than back to +/// the 31st. `chrono`'s `checked_add_months` does that clamping, matching +/// `_add_months` in the Python to the day. +fn advance_once(at: DateTime, recurrence: &str) -> Option> { + match recurrence { + "daily" => at.checked_add_signed(Duration::days(1)), + "weekly" => at.checked_add_signed(Duration::weeks(1)), + "monthly" => at.checked_add_months(Months::new(1)), + "yearly" => at.checked_add_months(Months::new(12)), + _ => None, + } +} + +/// The first fire time strictly after `after`, rolling past anything missed. +/// +/// A phone left in a drawer for a fortnight should not come back to fourteen +/// pending occurrences of the same daily reminder — it should come back to +/// tomorrow's. `None` when the rule is not one we know, which the caller reads as +/// "this reminder is finished". +pub fn next_occurrence( + remind_at: DateTime, + recurrence: &str, + after: DateTime, +) -> Option> { + let mut next = advance_once(remind_at, recurrence)?; + while next <= after { + match advance_once(next, recurrence) { + // The equality check is a guard against a step that does not move, + // which would spin here forever. It cannot happen with the four rules + // above; it is cheap insurance against a fifth that does not advance. + Some(step) if step != next => next = step, + _ => break, + } + } + Some(next) +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::TimeZone; + + fn utc(y: i32, m: u32, d: u32, h: u32, min: u32) -> DateTime { + Utc.with_ymd_and_hms(y, m, d, h, min, 0).unwrap() + } + + /// Mirrors `test_normalize_recurrence` in `tests/test_notes.py`. + #[test] + fn only_the_four_known_rules_are_recurrences() { + for rule in RECURRENCES { + assert_eq!(normalize(Some(rule)), Some(rule)); + } + assert_eq!(normalize(Some("none")), None); + assert_eq!(normalize(Some("")), None); + assert_eq!(normalize(Some("hourly")), None); + assert_eq!(normalize(None), None); + } + + /// Mirrors `test_next_occurrence_daily_weekly`. + #[test] + fn daily_and_weekly_keep_the_time_of_day() { + let base = utc(2026, 7, 1, 9, 0); + let after = utc(2026, 7, 1, 12, 0); + assert_eq!( + next_occurrence(base, "daily", after), + Some(utc(2026, 7, 2, 9, 0)) + ); + assert_eq!( + next_occurrence(base, "weekly", after), + Some(utc(2026, 7, 8, 9, 0)) + ); + } + + /// Mirrors `test_next_occurrence_skips_missed`. + #[test] + fn missed_occurrences_are_rolled_past_not_queued() { + let base = utc(2026, 7, 1, 9, 0); + let after = utc(2026, 7, 10, 12, 0); + assert_eq!( + next_occurrence(base, "daily", after), + Some(utc(2026, 7, 11, 9, 0)) + ); + } + + /// Mirrors `test_next_occurrence_monthly_clamps_month_end`. + #[test] + fn monthly_clamps_to_a_shorter_month() { + let base = utc(2026, 1, 31, 8, 0); + let after = utc(2026, 2, 1, 0, 0); + assert_eq!( + next_occurrence(base, "monthly", after), + Some(utc(2026, 2, 28, 8, 0)) + ); + } + + /// Mirrors `test_next_occurrence_yearly_and_none`. + #[test] + fn yearly_advances_a_year_and_an_unknown_rule_advances_nothing() { + let base = utc(2026, 3, 15, 7, 0); + let after = utc(2026, 3, 16, 0, 0); + assert_eq!( + next_occurrence(base, "yearly", after), + Some(utc(2027, 3, 15, 7, 0)) + ); + assert_eq!(next_occurrence(base, "none", after), None); + } + + /// Not in the Python suite, and the one that would bite hardest in practice: + /// a monthly reminder set on the 31st must not walk itself back to the 28th + /// permanently. Each step is taken from the ORIGINAL date, so February's clamp + /// does not become March's date. + #[test] + fn a_clamped_month_does_not_drag_later_months_back() { + let base = utc(2026, 1, 31, 8, 0); + // Far enough ahead that the loop takes several steps. + let after = utc(2026, 4, 15, 0, 0); + // Jan 31 -> Feb 28 -> Mar 28 -> Apr 28. The clamp is sticky once applied, + // which matches the server exactly — asserted so a future "fix" to either + // side has to change both. + assert_eq!( + next_occurrence(base, "monthly", after), + Some(utc(2026, 4, 28, 8, 0)) + ); + } + + /// A reminder completed before it was ever due still moves forward one step, + /// rather than staying put and firing again immediately. + #[test] + fn completing_early_still_advances() { + let base = utc(2026, 7, 10, 9, 0); + let after = utc(2026, 7, 1, 12, 0); + assert_eq!( + next_occurrence(base, "daily", after), + Some(utc(2026, 7, 11, 9, 0)) + ); + } +} diff --git a/core/src/local/store.rs b/core/src/local/store.rs index 1cf96b5..80fa92a 100644 --- a/core/src/local/store.rs +++ b/core/src/local/store.rs @@ -7,13 +7,14 @@ //! ("YYYY-MM-DDTHH:MM:SS.sssZ") so string ordering and date-range comparisons line //! up with the values the frontend sends. -use chrono::{Duration, SecondsFormat, Utc}; +use chrono::{DateTime, Duration, SecondsFormat, Utc}; use rusqlite::{params, params_from_iter, Connection, OptionalExtension}; use serde_json::Value; use uuid::Uuid; use crate::local::derive; use crate::local::models::*; +use crate::local::recur; fn now() -> String { Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true) @@ -529,9 +530,38 @@ pub fn update_note(conn: &Connection, id: &str, changes: &Value) -> rusqlite::Re load_note(conn, id) } +/// Mark a reminder handled. +/// +/// A recurring reminder advances to its next occurrence; a one-off clears both +/// `remind_at` AND `recurrence`. Clearing the rule as well matters: without it a +/// note whose recurrence is a value we do not recognise would keep that value +/// forever, invisible in every UI (they only render known rules) and waiting to +/// mean something the day the vocabulary grows. +/// +/// Same behaviour as the server's `POST //reminder/complete`, deliberately — +/// the same note can be completed from a browser or from a client, and a +/// disagreement here would move a reminder depending on which one you used. pub fn complete_reminder(conn: &Connection, id: &str) -> rusqlite::Result { - // Clear the reminder. (Recurrence advancement is a later refinement.) - conn.execute("UPDATE notes SET remind_at = NULL WHERE id = ?1", [id])?; + let note = load_note(conn, id)?; + let next = note + .remind_at + .as_deref() + .and_then(|at| DateTime::parse_from_rfc3339(at).ok()) + .and_then(|at| { + let rule = recur::normalize(note.recurrence.as_deref())?; + recur::next_occurrence(at.with_timezone(&Utc), rule, Utc::now()) + }); + + match next { + Some(at) => conn.execute( + "UPDATE notes SET remind_at = ?1 WHERE id = ?2", + params![at.to_rfc3339_opts(SecondsFormat::Millis, true), id], + )?, + None => conn.execute( + "UPDATE notes SET remind_at = NULL, recurrence = NULL WHERE id = ?1", + [id], + )?, + }; touch(conn, id)?; load_note(conn, id) }