feat(reminders): edit an existing reminder's description and time - #5507
feat(reminders): edit an existing reminder's description and time#5507evanhutnik wants to merge 2 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe reminder composer now supports creating and editing reminders. Edit mode loads existing values, validates descriptions, preserves unchanged fields, and submits minimal updates. Scheduling helpers provide current-time options and update patches. Next-soup actions, menus, and the 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
751a550 to
3c75a4e
Compare
The composer only ever created. A reminder's description and time were fixed once set — its name is not renamable through the entity-mutation path (the reminders API owns it), and nothing offered its schedule. Adds an edit mode over the same two steps, prefilled. The description step starts from what the reminder says and, unlike creating, cannot be left blank: the field was populated, so an empty one is a deletion rather than the create flow's "skip and derive from the entity". The date step leads with "Keep current time", so a rename costs one Enter instead of forcing a new date to be picked. That option is the only one exempt from the future filter, which is what keeps an overdue reminder renamable — keeping the time sends no schedule at all. Only changed fields are sent. An unchanged schedule has to be omitted rather than re-sent, or renaming a reminder that had already fired would be rejected for being in the past; an unchanged everything sends nothing, since the API rejects a patch with no fields. Rescheduling a reminder that was marked done also clears the flag — the dispatcher skips completed reminders, so the new time would otherwise silently never arrive. The component is renamed to ReminderComposerModal now that it does both.
Wires the composer's edit mode to the two surfaces a soup row is acted on from: the entity actions menu and the entity action hotkeys. 'r' is shared with Rename rather than given a key of its own. The two are mutually exclusive — a reminder row never offers Rename, since its `ownerId` is empty and the rename path explicitly refuses reminders — so sharing puts the edit where the muscle memory already is instead of leaving 'r' dead on a reminder. Recurring reminders are excluded. The composer only speaks one-shot schedules, so editing one through it would quietly collapse a cron into a single firing. Nothing in the product creates a recurring reminder today, so this excludes nothing reachable.
e9dc986 to
5e41299
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/features/next-soup/soup-view/create-soup-entity-actions.ts`:
- Around line 296-306: Update the Rename action eligibility, specifically the
logic used by makeRenameAction.canExecute(), to exclude entities whose type is
'reminder'. Preserve Rename for all other eligible entity types so reminder rows
show only the existing Edit reminder action.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc08a697-a223-4fc6-94d3-e9e024ba6591
📒 Files selected for processing (11)
apps/web/src/components/app/Layout.tsxapps/web/src/features/next-soup/actions/index.tsapps/web/src/features/next-soup/actions/make-edit-reminder-action.test.tsapps/web/src/features/next-soup/actions/make-edit-reminder-action.tsapps/web/src/features/next-soup/actions/use-entity-action-hotkeys.tsapps/web/src/features/next-soup/soup-view/create-soup-entity-actions.tsapps/web/src/features/reminders/ReminderComposerModal.tsxapps/web/src/features/reminders/reminder-composer.test.tsapps/web/src/features/reminders/reminder-composer.tsapps/web/src/features/reminders/reminder-schedule.test.tsapps/web/src/features/reminders/reminder-schedule.ts
| // A reminder never offers Rename — its name is its description, which the | ||
| // reminders API owns — so this takes that slot, and the same 'r' key. | ||
| // Single-entity only: the editor asks about one reminder's time. | ||
| if (entities.length === 1 && editReminderAction.canExecute(entities[0])) { | ||
| middleItems.push({ | ||
| id: 'edit-reminder', | ||
| label: 'Edit reminder', | ||
| hotkeyToken: TOKENS.entity.action.rename, | ||
| onClick: handle(editReminderAction.executeWithSoup), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the rename action eligibility rules for reminder entities.
fd -t f 'make-rename-action.ts' --exec cat -n {}Repository: macro-inc/macro
Length of output: 2467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/web/src/features/next-soup/soup-view/create-soup-entity-actions.ts"
echo "### file exists and relevant line range"
wc -l "$file"
sed -n '260,315p' "$file" | nl -ba -v260
echo
echo "### related action definitions/usages in file"
rg -n "renameAction|editReminderAction|create-soup-action|canExecute|executeWithSoup|action" "$file"Repository: macro-inc/macro
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/web/src/features/next-soup/soup-view/create-soup-entity-actions.ts"
echo "### relevant line range"
sed -n '260,315p' "$file" | awk '{printf "%5d\t%s\n", NR+259, $0}'
echo
echo "### related action definitions/usages in file"
rg -n "renameAction|editReminderAction|create-soup-action|canExecute|executeWithSoup|action" "$file"
echo
echo "### imports/top"
sed -n '1,80p' "$file" | awk '{printf "%5d\t%s\n", NR, $0}'Repository: macro-inc/macro
Length of output: 9523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### locate action files by name"
fd -t f 'make-.*action\.ts|actions' apps/web/src/features/next-soup | sort
echo
echo "### rename action import target"
rg -n "export const makeRenameAction|makeRenameAction" apps/web/src/features/next-soup -g '*.ts'
echo
echo "### edit reminder action"
rg -n "export const makeEditReminderAction|makeEditReminderAction|canExecute" apps/web/src/features/next-soup -g '*.ts' -A8 -B8 | head -220
echo
echo "### hotkey mapping around action rename/edit reminder"
rg -n "TOKENS\.entity\.action\.rename|registerHotkey|entity/action/rename|editReminder|reminder" apps/web/src/features apps/web/src/components apps/web/src -g '*.ts' -g '*.tsx' -A10 -B10 | head -300Repository: macro-inc/macro
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### make-edit-reminder-action.ts"
sed -n '1,180p' apps/web/src/features/next-soup/actions/make-edit-reminder-action.ts | awk '{printf "%5d\t%s\n", NR, $0}'
echo
echo "### use-entity-action-hotkeys.ts rename/reminder handling"
rg -n "TOKENS\.entity\.action\.rename|renameAction|editReminderAction|remindersActive|reminder" apps/web/src/features/next-soup/actions/use-entity-action-hotkeys.ts -A10 - B10
echo
echo "### simple rename eligibility model"
python3 - <<'PY'
types = ["reminder", "document", "email", "channel", "channel_message", "foreign", "project"]
owned = True
for t in types:
result = False if t in {"email", "channel_message", "channel_thread", "foreign"} else owned
if t == "channel":
result = False
print(f"{t}: {result}")
PYRepository: macro-inc/macro
Length of output: 11592
Make Rename and Edit reminder mutually exclusive in the menu
makeRenameAction.canExecute() still returns true for reminder entities, so the Rename item can appear alongside Edit reminder and both use TOKENS.entity.action.rename. Add entity.type === 'reminder' to the rename eligibility or another exclusion that keeps reminder rows from showing the Rename action.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/features/next-soup/soup-view/create-soup-entity-actions.ts`
around lines 296 - 306, Update the Rename action eligibility, specifically the
logic used by makeRenameAction.canExecute(), to exclude entities whose type is
'reminder'. Preserve Rename for all other eligible entity types so reminder rows
show only the existing Edit reminder action.
A reminder was write-once from the UI. The
PATCH /reminders/{id}endpoint has been there since the reminders API landed, but nothing reached it — a reminder's name is its description, which the entity-mutation rename path explicitly refuses, and no surface offered its schedule. A typo or a bad time meant deleting and re-creating.What this adds
The composer gains an edit mode over the same two steps it already asks, prefilled from the row:
3d,tomorrow 3pm) as before.Reachable from a reminder row in Soup, on the entity actions menu (Edit reminder) and on
r.Notes on the details
ris shared with Rename. The two are mutually exclusive — a reminder row never offers Rename, since itsownerIdis empty andgetEntityRenameDatarefuses reminders outright — so sharing the key puts the edit where the muscle memory is instead of leavingrdead on a reminder.Only changed fields are sent. An unchanged schedule is omitted rather than re-sent: re-sending the time of a reminder that has already fired would be rejected for being in the past, so omitting it is what keeps an overdue reminder renamable. "Keep current time" is likewise the one option exempt from the future filter. An unchanged everything sends no request — the API rejects a patch with no fields.
Rescheduling a done reminder clears the flag.
due_firingsskips completed reminders, so giving one a new time without also sendingcompleted: falsewould look like it worked and then silently never arrive. A description-only edit leaves the flag alone — renaming something you finished is not a request for it to happen again.Recurring reminders are excluded. The composer only speaks one-shot schedules, so editing one through it would quietly collapse a cron into a single firing. Nothing in the product creates a recurring reminder today, so this excludes nothing a user can reach.
CreateReminderModalis renamed toReminderComposerModalnow that it does both.