Skip to content

fix(reminders): replace a journal or note reminder instead of adding a second - #1943

Draft
h4yfans wants to merge 7 commits into
mainfrom
journal-reminder-edit-remove
Draft

fix(reminders): replace a journal or note reminder instead of adding a second#1943
h4yfans wants to merge 7 commits into
mainfrom
journal-reminder-edit-remove

Conversation

@h4yfans

@h4yfans h4yfans commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

A reader set a reminder on a journal entry with the "in one week" preset, then wanted a different hour. There was no way to change or remove it, so he opened the picker again and chose a custom time, expecting it to override. It did not. He ended up with two reminders on one entry, both firing.

Two defects sat behind that. use-journal-reminders.ts called useCreateReminder unconditionally, so every pick appended a row. journal-reminder-button.tsx wired only onSelect into ReminderPicker, even though the picker has taken reminders, onEdit and onDelete since the task surface shipped, and even though the journal hook already exposed deleteReminder. The tooltip could already count the extras (reminder.tooltipMore) while offering no way to remove one. note-reminder-button.tsx and the note page's own toolbar picker had the identical gap, so both are fixed here.

A journal entry or a note now carries one reminder. useSetOrReplaceReminder in hooks/use-set-or-replace-reminder.ts holds that rule once for both surfaces: given the id of the active reminder it updates that row, and with null it creates. It reuses the existing reminders.update channel, so nothing in packages/contracts or main/ipc/reminder-handlers.ts changed. Updating in place rather than deleting and recreating keeps the row id, which means sync sees an update and the calendar projection follows the same row; remindersService.updateReminder already resets status to pending and clears triggeredAt/snoozedUntil when remindAt moves, so replacing a snoozed reminder re-arms it. The note carried by the new pick is written through as null when the picker comes back without one, so replacing clears a stale note instead of stranding it.

The note surface merges the note's own reminders with its highlight reminders. Only the note's own next active reminder is replaceable there: moving a passage reminder from the note toolbar would silently reschedule something the user never opened. Highlight reminders stay in the list with their own remove button.

There is no migration. Entries that already collected duplicates keep every row, they still fire, and the picker now lists each one with its own remove button, which is how a user gets rid of them. Task reminders are deliberately untouched: that surface already had edit and remove, and its +N affordance treats several reminders on one task as intentional.

Blast radius is the two reminder buttons, the note page's toolbar picker, the two hooks and two English locale files. ReminderPicker itself is unchanged; it already rendered the management list when handed the props. Four test files that partially mock these hooks were extended with the new activeReminders and editReminder members.

Carries the test-only main fix from the calendar-widget PR until it lands; rebasing after that drops it.

Closes #1939

Release note

Changing the time on a journal or note reminder now moves the reminder you already had instead of adding a second one, and the picker lets you remove it.

Test plan

Check Outcome
vitest --project renderer on the 4 touched specs, against origin/main source 15 failed, 8 passed (red first)
same 4 specs with the fix 23 passed
pnpm --filter @memry/desktop test:renderer 704/705 files passed. The one failure, home/widgets/calendar-widget-refresh.test.tsx, failed identically with every source file of this branch reverted to origin/main, so it predates this work. The carried commit fixes it: that spec is now 6 passed
pnpm --filter @memry/desktop test:main 566 files, 7735 tests pass
pnpm --filter @memry/desktop typecheck:web pass
pnpm --filter @memry/desktop typecheck:node pass
pnpm --filter @memry/desktop typecheck:test pass
pnpm lint pass, 0 errors
pnpm --filter @memry/desktop i18n:check pass
pnpm check:architecture pass
pnpm check:contracts pass
pnpm docs:impact --base origin/main --strict covered
pnpm docs:build pass
git diff --check clean

pnpm ipc:generate && pnpm ipc:check was not run: no contract, preload, main IPC handler or generated binding changed. The replace path reuses the existing reminder:update channel.

E2E, tests/e2e/journal-reminder-edit.e2e.ts. It navigates to the journal, sets the "In 1 Week" preset through the real picker, reopens it, picks a custom day and 18:45 from the date and time pane, and reads the rows back over window.api.reminders.list. It asserts exactly one row remains, that its id is the preset's id, that its time moved, and that the local parts are the picked day at 18:45. Then it removes the reminder from the picker and asserts the row count is zero and the bell's accessible name is back to "Set reminder to revisit".

Green with the fix, 1 passed (25.3s). Red with the seven changed source files checked out from origin/main:

Error: expect(locator).toHaveCount(expected) failed
Locator: locator('[data-slot="picker-content"]').getByRole('button', { name: 'Delete reminder' })
Expected: 1
Received: 0
> 81 |     await expect(picker.getByRole('button', { name: 'Delete reminder' })).toHaveCount(1)
1 failed

That is the dead end from the report: an entry with a reminder, and the picker offering no way to remove it. The duplicate half of the bug is pinned by the renderer specs, where reverting the fix turns 10 of them red.

Five mutations were run against the staged fix and each was killed: always create rather than replace (10 tests), dropping the note write-through on replace (1), removing onDelete from the journal picker (2), replacing from the merged note and highlight list rather than the note's own (2), and replacing the raw first row instead of the next active one (3). The baseline returned 23 passed.

I grepped apps/desktop/tests/e2e/ for specs asserting the picker layout. Only integration.e2e.ts mentions reminders, and its selectors (add-reminder, reminder-picker, reminder-1hour) exist nowhere in the renderer; every assertion is behind an isVisible() guard and the tests end in expect(true).toBe(true). It cannot be affected by this change. property-status-options.e2e.ts matches [data-slot="picker-content"] but drives the status picker, and no Picker primitive changed.

@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation test labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

React Doctor found 1 new issue in 1 file · 1 warning · score 84 / 100 (Needs work) · 1 fixed · vs main

1 warning

src/renderer/src/pages/note.tsx

  • ⚠️ L173 React function has high control-flow complexity no-high-complexity-react-function

Reviewed by React Doctor for commit 4aea273. See inline comments for fixes.

The calendar widget refresh suite faked the clock onto 2026-08-31, the day it was
written. use-today snapshots the local date into module scope at import and re-reads
the wall clock for its first subscriber, so on every later day that fake clock arrives
as a midnight rollover. todayCalendarRange moves, the useCalendarRange query key moves
with it, and the widget fetches a second day during mount, which is the second
getRange call the first test counted.

Derive the fixture clock and its event hours from the real local date instead, so the
mount no longer straddles a day boundary. Local date fields rather than a UTC instant,
because far enough from UTC the two name different days.
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.05263% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
apps/desktop/src/renderer/src/pages/note.tsx 33.33% 2 Missing ⚠️
...sktop/src/renderer/src/hooks/use-note-reminders.ts 96.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@h4yfans

h4yfans commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Dismissing the one new warning as a diff-attribution artifact, with a disproof.

no-high-complexity-react-function at apps/desktop/src/renderer/src/pages/note.tsx:173 (NotePage) already fires on main. I checked that single file out from origin/main in this worktree and ran the same tool:

$ git checkout origin/main -- apps/desktop/src/renderer/src/pages/note.tsx
$ npx react-doctor@latest apps/desktop/src/renderer/src/pages/note.tsx
⚠ React function has high control-flow complexity
  react-doctor/no-high-complexity-react-function
  src/renderer/src/pages/note.tsx:173

Same rule, same file, same line, without any of this branch's changes. NotePage is a pre-existing 1400 line component and this PR adds five lines to it: one destructured field from useNoteReminders and two callback props on the reminder picker. None of them is a branch, so the function's control flow is unchanged.

Splitting NotePage would be a real improvement and it is worth its own PR. Doing it here would bury a reminder fix under a page refactor, so I am leaving it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: A journal reminder cannot be edited or removed, and a custom time adds a second one

1 participant