feat: add /remind command with natural language time parsing#153
feat: add /remind command with natural language time parsing#153BillChirico merged 15 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a personal reminder system enabling users to create, list, and cancel reminders with natural language time parsing, recurring cron support, snooze buttons, and database persistence. Integrates with the scheduler for polling and includes configuration management, migrations, and comprehensive test coverage. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
Claude finished @BillChirico's task in 3m 23s —— View job Code Review — PR #153: feat: add /remind command
2 issues remaining (0 critical, 2 warnings). Most feedback from previous rounds has been addressed — nice iteration.
Verdict: Request changes. See inline comment and review body for details and fix instructions. |
Review Summary5 issues found across the PR (0 critical, 4 warnings, 1 nitpick).
Not flagged (acceptable): Documentation (AGENTS.md, README) not updated — required per AGENTS.md conventions for new commands/modules/tables, but can be done as a follow-up. 📋 AI-friendly fix prompt (click to expand) |
There was a problem hiding this comment.
5 issues found: reminders.enabled defaults to true (should be false per convention), missing 'remind' in permissions.allowedCommands, checkReminders() missing null pool check, missing reminders schema in CONFIG_SCHEMA for API validation, and dead maxRecurring config. See inline comments and summary comment for details.
There was a problem hiding this comment.
Pull request overview
Adds a new /remind feature to the bot, introducing personal reminders with a zero-dependency natural-language time parser and a scheduler-driven delivery loop.
Changes:
- Implement
/remindcommand (me/list/cancel) backed by a newremindersDB table. - Add a natural language time parser utility and comprehensive tests.
- Integrate reminder polling into the existing scheduler and register button handlers for snooze/dismiss actions.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
migrations/015_reminders.cjs |
Adds reminders table and a due-reminders partial index. |
src/utils/timeParser.js |
Implements zero-dependency parsing for relative/weekday/tomorrow formats. |
src/modules/reminderHandler.js |
Fires due reminders, builds snooze/dismiss buttons, handles button interactions. |
src/commands/remind.js |
Implements /remind me, /remind list, /remind cancel. |
src/modules/scheduler.js |
Calls checkReminders() from the existing 60s polling loop. |
src/modules/events.js |
Registers an InteractionCreate handler for reminder buttons. |
src/api/utils/configAllowlist.js |
Allowlists reminders config for API usage. |
src/api/utils/configValidation.js |
Formatting-only change to error push line wrapping. |
src/modules/welcomeOnboarding.js |
Formatting-only change (multi-line ternaries). |
config.json |
Adds reminders config section and minor prompt/message encoding changes. |
tests/utils/timeParser.test.js |
Adds unit tests for time parsing and parsing+message splitting. |
tests/modules/reminderHandler.test.js |
Adds tests for firing, fallback delivery, snooze, dismiss, and recurring reschedule behavior. |
tests/commands/remind.test.js |
Adds tests for command flow, config gate, limits, list, cancel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
7 issues found (0 critical, 6 warnings, 1 nitpick). See inline comments for details.
Summary of issues:
- 🟡
config.json: Missingremindentry inpermissions.allowedCommands— every other command has one (AGENTS.md step 5) - 🟡
src/modules/reminderHandler.js:12: Circular dependency betweenscheduler.js↔reminderHandler.js— works at runtime but fragile - 🟡
src/modules/reminderHandler.js:141: Reminder marked completed even when notification delivery fails silently (both DM + channel fail) - 🟡
src/modules/reminderHandler.js:186:handleReminderSnoozemissing pool null check — crashes if DB unavailable - 🟡
src/modules/reminderHandler.js:239:handleReminderDismissmissing pool null check — crashes if DB unavailable - 🟡
migrations/015_reminders.cjs:32: Missing composite index on(guild_id, user_id) WHERE completed = falsefor per-user queries - 🔵
config.json:267: Dead configmaxRecurring— defined but never enforced
Not flagged: AGENTS.md/README documentation updates — required per conventions but acceptable as follow-up.
|
| Filename | Overview |
|---|---|
| src/modules/reminderHandler.js | reminder polling, notification delivery, and button handlers — missing try/catch blocks around database queries violate error-handling rule |
| src/commands/remind.js | slash command handler for /remind me/list/cancel — proper error handling with transactions and advisory locks for race condition prevention |
| src/utils/timeParser.js | zero-dependency natural language time parser supporting shorthand, "in N unit", "tomorrow", and "next " formats with comprehensive test coverage |
| migrations/015_reminders.cjs | creates reminders table with appropriate partial indexes on (remind_at) and (guild_id, user_id) for active reminders |
| src/modules/scheduler.js | adds checkReminders() call to 60-second polling loop |
| src/modules/events.js | registers reminder snooze/dismiss button handler with proper error handling wrapper |
Last reviewed commit: 1bd6271
There was a problem hiding this comment.
3 issues found (0 critical, 3 warnings, 1 nitpick). Most issues from previous reviews have been addressed. Remaining:
- 🟡
config.json: Missingremindentry inpermissions.allowedCommands— defaults to admin-only when permissions enabled - 🟡
src/commands/remind.js:30: NosetMaxLengthonmessagestring option — reminder delivery can fail if message exceeds embed 4096-char limit - 🟡
src/modules/reminderHandler.js:143:checkRemindersdoesn't respect per-guildreminders.enabled— fires reminders for guilds that disabled the feature - 🔵
src/api/utils/configValidation.js:149:maxRecurringin schema but removed from config.json — dead schema property
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Implement personal reminder system with: - /remind me <time> <message> — set reminders with natural language - /remind list — view active reminders in an embed - /remind cancel <id> — cancel owned reminders Features: - Zero-dependency natural language time parser (src/utils/timeParser.js) supports: 'in 5 minutes', 'tomorrow at 3pm', 'next monday', '5m', etc. - Snooze buttons (15m, 1h, Tomorrow, Dismiss) on fired reminders - DM delivery with channel mention fallback - Recurring reminders via cron expressions - Per-user reminder limits (configurable, default 25) - Config gating via config.reminders.enabled - Persistent snooze across bot restarts (DB-backed) New files: - migrations/015_reminders.cjs — reminders table + partial index - src/commands/remind.js — slash command handler - src/utils/timeParser.js — natural language time parser - src/modules/reminderHandler.js — check/fire/snooze/dismiss logic Integration: - Scheduler polls reminders every 60s tick - Button handler registered in events.js - 'reminders' added to SAFE_CONFIG_KEYS allowlist - config.json updated with reminders section Tests: 65 new tests (timeParser: 41, reminderHandler: 14, command: 10) Closes #137
…hen DB unavailable
- Import getConfig from config.js - Check if reminders are disabled for a guild before processing - Skip reminders that belong to guilds with reminders disabled - This prevents pending reminders from firing if guild admin disables feature
- Add .setMaxLength(200) to 'when' option (e.g., 'tomorrow at 3pm') - Add .setMaxLength(500) to 'message' option - Prevents oversized inputs that would exceed Discord embed description limit (4096 chars) - Provides better validation at input time
- Add idx_reminders_user composite index on (guild_id, user_id, completed) - Improves performance for frequent /remind list, /remind cancel, and count queries - These queries filter by (guild_id, user_id) with completed=false - Complements existing idx_reminders_user_active filtered index
- Update 'tomorrow at' regex to capture '3 pm' and '3:30 am' (with spaces) - Update 'next <day> at' regex to capture spaced am/pm suffix - Change regex from (.+?) to (.+?(?:\s+[ap]m)?) for time capture groups - Add tests for 'tomorrow at 3 pm', 'tomorrow at 9:30 am' - Add tests for 'next fri at 9 am', 'next monday at 2:30 pm' - Fixes issue where spaced am/pm was not properly captured
- Remove maxRecurring property from reminders schema - This property was removed from config.json but remained in schema - Schema now only validates enabled and maxPerUser properties - Prevents validation errors for removed config option
c9bf3a3 to
bd015e0
Compare
| const { rows } = await pool.query( | ||
| 'SELECT * FROM reminders WHERE completed = false AND remind_at <= NOW()', | ||
| ); |
There was a problem hiding this comment.
wrap in try/catch per error-handling rule — if query fails, error bubbles up without context
| const { rows } = await pool.query( | |
| 'SELECT * FROM reminders WHERE completed = false AND remind_at <= NOW()', | |
| ); | |
| try { | |
| const { rows } = await pool.query( | |
| 'SELECT * FROM reminders WHERE completed = false AND remind_at <= NOW()', | |
| ); | |
| for (const reminder of rows) { | |
| // ... existing reminder processing ... | |
| } | |
| } catch (err) { | |
| logError('Failed to fetch due reminders', { | |
| error: err instanceof Error ? err.message : String(err), | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reminderHandler.js
Line: 143-145
Comment:
wrap in try/catch per error-handling rule — if query fails, error bubbles up without context
```suggestion
try {
const { rows } = await pool.query(
'SELECT * FROM reminders WHERE completed = false AND remind_at <= NOW()',
);
for (const reminder of rows) {
// ... existing reminder processing ...
}
} catch (err) {
logError('Failed to fetch due reminders', {
error: err instanceof Error ? err.message : String(err),
});
}
```
How can I resolve this? If you propose a fix, please make it concise.| return; | ||
| } | ||
|
|
||
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); |
There was a problem hiding this comment.
wrap in try/catch per error-handling rule
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); | |
| try { | |
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reminderHandler.js
Line: 240
Comment:
wrap in try/catch per error-handling rule
```suggestion
try {
const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]);
```
How can I resolve this? If you propose a fix, please make it concise.| await pool.query( | ||
| 'UPDATE reminders SET remind_at = $1, completed = false, snoozed_count = snoozed_count + 1 WHERE id = $2', | ||
| [newRemindAt.toISOString(), reminderId], | ||
| ); |
There was a problem hiding this comment.
wrap in try/catch per error-handling rule
| await pool.query( | |
| 'UPDATE reminders SET remind_at = $1, completed = false, snoozed_count = snoozed_count + 1 WHERE id = $2', | |
| [newRemindAt.toISOString(), reminderId], | |
| ); | |
| try { | |
| await pool.query( | |
| 'UPDATE reminders SET remind_at = $1, completed = false, snoozed_count = snoozed_count + 1 WHERE id = $2', | |
| [newRemindAt.toISOString(), reminderId], | |
| ); | |
| } catch (err) { | |
| logError('Failed to snooze reminder', { | |
| reminderId, | |
| userId: interaction.user.id, | |
| error: err instanceof Error ? err.message : String(err), | |
| }); | |
| await interaction.reply({ | |
| content: '❌ Failed to snooze reminder. Please try again.', | |
| ephemeral: true, | |
| }); | |
| return; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reminderHandler.js
Line: 266-269
Comment:
wrap in try/catch per error-handling rule
```suggestion
try {
await pool.query(
'UPDATE reminders SET remind_at = $1, completed = false, snoozed_count = snoozed_count + 1 WHERE id = $2',
[newRemindAt.toISOString(), reminderId],
);
} catch (err) {
logError('Failed to snooze reminder', {
reminderId,
userId: interaction.user.id,
error: err instanceof Error ? err.message : String(err),
});
await interaction.reply({
content: '❌ Failed to snooze reminder. Please try again.',
ephemeral: true,
});
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return; | ||
| } | ||
|
|
||
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); |
There was a problem hiding this comment.
wrap in try/catch per error-handling rule
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); | |
| try { | |
| const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reminderHandler.js
Line: 309
Comment:
wrap in try/catch per error-handling rule
```suggestion
try {
const { rows } = await pool.query('SELECT * FROM reminders WHERE id = $1', [reminderId]);
```
How can I resolve this? If you propose a fix, please make it concise.| return; | ||
| } | ||
|
|
||
| await pool.query('UPDATE reminders SET completed = true WHERE id = $1', [reminderId]); |
There was a problem hiding this comment.
wrap in try/catch per error-handling rule
| await pool.query('UPDATE reminders SET completed = true WHERE id = $1', [reminderId]); | |
| try { | |
| await pool.query('UPDATE reminders SET completed = true WHERE id = $1', [reminderId]); | |
| } catch (err) { | |
| logError('Failed to dismiss reminder', { | |
| reminderId, | |
| userId: interaction.user.id, | |
| error: err instanceof Error ? err.message : String(err), | |
| }); | |
| await interaction.reply({ | |
| content: '❌ Failed to dismiss reminder. Please try again.', | |
| ephemeral: true, | |
| }); | |
| return; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/reminderHandler.js
Line: 323
Comment:
wrap in try/catch per error-handling rule
```suggestion
try {
await pool.query('UPDATE reminders SET completed = true WHERE id = $1', [reminderId]);
} catch (err) {
logError('Failed to dismiss reminder', {
reminderId,
userId: interaction.user.id,
error: err instanceof Error ? err.message : String(err),
});
await interaction.reply({
content: '❌ Failed to dismiss reminder. Please try again.',
ephemeral: true,
});
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Personal reminder system with natural language time parsing, recurring reminders, and snooze buttons.
New Commands
/remind me <time> <message>— set a reminder with natural language time/remind list— view active reminders in an embed/remind cancel <id>— cancel an owned reminderFeatures
in 5 minutes,tomorrow at 3pm,next monday,5m,2h,30s, etc.config.reminders.enabledcontrols the featureNew Files
migrations/015_reminders.cjsremind_atsrc/commands/remind.jssrc/utils/timeParser.jssrc/modules/reminderHandler.jsIntegration Points
events.jsremindersadded toSAFE_CONFIG_KEYSallowlistconfig.jsonupdated withreminderssectionTests
65 new tests across 3 test files:
tests/utils/timeParser.test.js— 41 tests (edge cases, all formats)tests/modules/reminderHandler.test.js— 14 tests (fire, snooze, dismiss, recurring)tests/commands/remind.test.js— 10 tests (create, list, cancel, limits, config gate)All 2712 tests pass. Zero lint errors.
Closes #137