Conversation
📝 WalkthroughWalkthroughAdds a birthday reminder subsystem: new DB table/model/queries, commands to create/remove/test reminders, scheduler logic to send daily DM reminders at 00:00 UTC, and updates to birthday-set behavior and user attribute handling. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as Birthday Scheduler (Daily 00:00 UTC)
participant DB as Database / BirthdayReminderModel
participant Discord as Discord API
participant Notifier as Notifier User
Scheduler->>DB: getPendingReminders(currentUTCYear)
DB-->>Scheduler: pending reminder rows
loop per reminder
Scheduler->>Scheduler: compute next occurrence & daysUntil
alt daysUntil == entry.daysBefore
Scheduler->>Discord: fetch tracked user & notifier
Discord-->>Scheduler: user objects (or null)
Scheduler->>Notifier: send DM (embed)
alt DM success
Notifier-->>Scheduler: OK
Scheduler->>DB: markReminderSent(notifierId, trackedUserId, year)
DB-->>Scheduler: updated
else DM failed
Notifier-->>Scheduler: Error
Scheduler->>Scheduler: log failure
end
else
Scheduler->>Scheduler: skip
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
commands/birthday_testreminder.js (1)
16-18: Filter reminders in SQL instead of loading all pending rows.
getPendingReminders(currentYear)+ in-memory filter pulls unnecessary rows. Add a notifier-scoped query (e.g.,WHERE notifier_id = ?) to reduce load and data exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/birthday_testreminder.js` around lines 16 - 18, The current code calls this.client.db.birthdayReminder.getPendingReminders(currentYear) and then does an in-memory filter (pending.filter((r) => r.notifierId === notifierId)), which loads unnecessary rows; change the DB call to perform the filter in SQL instead. Add or update a method on this.client.db.birthdayReminder (e.g., getPendingRemindersForNotifier or extend getPendingReminders to accept notifierId) to query pending reminders with WHERE notifier_id = ? AND year = ? (or equivalent), then replace the pending + mine in-memory filtering with a single call to that new/updated method and remove the pending.filter usage. Ensure you pass notifierId into the new DB method and update call sites accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/birthdayScheduler.js`:
- Around line 102-109: The reminder is marked sent regardless of send success;
move the await
this.client.db.birthdayReminder.markReminderSent(entry.notifierId,
entry.trackedUserId, currentYear) into the try block immediately after await
notifier.send({ embeds: [reminderEmbed] }) so it only runs when notifier.send
succeeds, and leave the catch to log the dmError via logError (do not call
markReminderSent in the catch).
- Around line 71-79: The nextBirthday calculation currently rebuilds a date at
00:00:00Z which discards the stored UTC time offset (birthdays were saved via
birthday.toISOString() in commands/birthday_set.js), so change the logic in
classes/birthdayScheduler.js to preserve the time component from the parsed
birthday Date (the variable birthday) when deriving the candidate for the
current year: construct thisYear by cloning birthday but setting its year to
currentYear (keeping month, day, and time), compare that to now, and if it's in
the past increment the year by one to form nextBirthday; ensure you use
UTC-getters/setters (e.g., getUTCFullYear/setUTCFullYear) so the original stored
UTC time is preserved when computing daysUntil.
- Line 61: The cron job scheduled in cron.schedule('0 0 * * *', async () => {
... } ) relies on node-cron defaulting to system timezone; update the call to
pass an options object with timezone: 'UTC' (e.g., cron.schedule('0 0 * * *',
async () => { ... }, { timezone: 'UTC' })) so the "midnight UTC" behavior is
enforced; locate the call to cron.schedule in birthdayScheduler.js and add the {
timezone: 'UTC' } option.
In `@commands/birthday_set.js`:
- Around line 129-131: In the catch block that currently calls logError(...) and
await interaction.editReply(...), keep the detailed error only in the log
(logError) and replace the user-facing text passed to interaction.editReply in
the birthday_set command with a generic message (e.g., "An unexpected error
occurred while setting your birthday. Please try again later."); do not include
error.message or any internal details in the reply. Ensure the catch block still
logs the full error via logError and only sends the sanitized generic message to
the user.
- Around line 107-117: The review flags that you are logging sensitive full
birth date information via the variables day, month, year, timezone and the
constructed birthday ISO; update the code in the birthday_set.js block where
log(...) prints "Received inputs..." and "Constructed birthday..." so it no
longer emits raw PII: remove or redact those log calls and instead log only
non-identifying status messages (e.g., "Received birthday inputs for user"
without the date) and a success message that omits the ISO timestamp; keep the
existing validation (Number.isNaN(birthday.getTime())), user feedback
(interaction.editReply), and persistence call
(this.client.db.user.setUserAttr(userId, 'birthdays', birthday.toISOString()))
but ensure logs around these actions reference only userId or operation status,
not the birthday, day, month, year, or timezone variables.
In `@commands/birthday_testreminder.js`:
- Around line 10-49: The execute override in this command bypasses the
DevCommand access check (isDev) so non-devs can run it; fix by routing through
the DevCommand guard: at the top of this command's execute method call await
super.execute(interaction) (or explicitly call the DevCommand isDev guard and
return when it denies access) before running the test DM logic; reference the
execute method on this class, the base DevCommand class and its isDev guard to
ensure the original access-control path is invoked and non-dev users are
blocked.
In `@database/models/BirthdayReminderModel.js`:
- Around line 9-35: The upsertReminder, deleteReminder and markReminderSent
functions currently log success unconditionally even when this.db.executeQuery
can swallow failures; update each to detect failures from executeQuery (either
by checking the returned result object for an error/ok flag or by letting the
underlying error propagate) and rethrow or return an explicit error when
persistence failed, only calling log(...) after a confirmed successful result;
reference the methods upsertReminder, deleteReminder, markReminderSent and the
database executeQuery behavior (see Database.js executeQuery) to implement
either: (a) throw when result indicates failure, or (b) remove internal
swallowing in executeQuery and let exceptions bubble, and ensure logging happens
after success-only confirmation.
---
Nitpick comments:
In `@commands/birthday_testreminder.js`:
- Around line 16-18: The current code calls
this.client.db.birthdayReminder.getPendingReminders(currentYear) and then does
an in-memory filter (pending.filter((r) => r.notifierId === notifierId)), which
loads unnecessary rows; change the DB call to perform the filter in SQL instead.
Add or update a method on this.client.db.birthdayReminder (e.g.,
getPendingRemindersForNotifier or extend getPendingReminders to accept
notifierId) to query pending reminders with WHERE notifier_id = ? AND year = ?
(or equivalent), then replace the pending + mine in-memory filtering with a
single call to that new/updated method and remove the pending.filter usage.
Ensure you pass notifierId into the new DB method and update call sites
accordingly.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57332bad-e19e-4386-908a-61791e81d541
📒 Files selected for processing (13)
classes/birthdayScheduler.jscommands/birthday_notify.jscommands/birthday_set.jscommands/birthday_testreminder.jscommands/birthday_unnotify.jscommands/commandgroups/birthday.jsdatabase/Database.jsdatabase/models/BirthdayReminderModel.jsdatabase/models/UserModel.jsdatabase/models/index.jsdatabase/queries/birthdayReminderQueries.jsdatabase/tables/birthdayReminderTable.jsdatabase/tables/index.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
database/models/BirthdayReminderModel.js (1)
18-26: Inconsistent return value.
deleteReminderreturnsresultwhileupsertReminderandmarkReminderSentreturn nothing. If the return value isn't needed by callers, consider removing it for consistency.♻️ Suggested fix
async deleteReminder(notifierId, trackedUserId) { const query = birthdayReminderQueries.DELETE_REMINDER; const result = await this.db.executeQuery(query, [notifierId, trackedUserId]); if (!result.changes) { throw new Error(`Failed to delete birthday reminder: notifier=${notifierId}, tracked=${trackedUserId}`); } log(`Deleted birthday reminder: notifier=${notifierId}, tracked=${trackedUserId}`); - return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/models/BirthdayReminderModel.js` around lines 18 - 26, deleteReminder currently returns the execution result while upsertReminder and markReminderSent return nothing; remove the inconsistent return by deleting the final "return result;" in the deleteReminder method (in BirthdayReminderModel.deleteReminder) so the method behaves consistently (still throw on !result.changes and still log the deletion); ensure no callers rely on the returned value and adjust callers if any do.commands/birthday_set.js (1)
88-90: Extract duplicatedmonthNamesinto a single constant.The same month array is declared twice (Line 88 and Line 118). Pull it into one module-level constant to avoid drift and simplify maintenance.
Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/birthday_set.js` around lines 88 - 90, Extract the duplicated monthNames array into a single module-level constant (e.g., const MONTH_NAMES) and replace both local declarations with references to that constant; update any uses in the file (such as in the birthday validation logic where monthNames is referenced) to use MONTH_NAMES and remove the second array declaration to avoid drift and duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/birthday_set.js`:
- Around line 48-49: The command accepts minute-offset timezones (e.g., +5.30)
but downstream matching in database/queries/userQueries.js uses
strftime('%m-%dT%H', birthdays) (hour-only), so users with non-:00 minute
offsets get wrong notifications; either reject minute offsets at input or make
matching minute-accurate. Fix by updating commands/birthday_set.js timezone
argument validation (the timezone option and the code that builds the ISO
datetime stored) to reject or normalize any timezone strings with non-00 minutes
(e.g., allow only offsets whose minutes are 00) and return a clear validation
error to the user, or alternatively update the matching logic in
database/queries/userQueries.js to use strftime('%m-%dT%H:%M', birthdays) (and
corresponding scheduler/job logic) so minutes are matched end-to-end; choose one
approach and make the change consistently where timezone ISO is constructed and
where strftime is used.
---
Nitpick comments:
In `@commands/birthday_set.js`:
- Around line 88-90: Extract the duplicated monthNames array into a single
module-level constant (e.g., const MONTH_NAMES) and replace both local
declarations with references to that constant; update any uses in the file (such
as in the birthday validation logic where monthNames is referenced) to use
MONTH_NAMES and remove the second array declaration to avoid drift and
duplication.
In `@database/models/BirthdayReminderModel.js`:
- Around line 18-26: deleteReminder currently returns the execution result while
upsertReminder and markReminderSent return nothing; remove the inconsistent
return by deleting the final "return result;" in the deleteReminder method (in
BirthdayReminderModel.deleteReminder) so the method behaves consistently (still
throw on !result.changes and still log the deletion); ensure no callers rely on
the returned value and adjust callers if any do.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdff4ca5-b3fd-410e-b99d-e7c36c719347
📒 Files selected for processing (4)
classes/birthdayScheduler.jscommands/birthday_set.jscommands/birthday_testreminder.jsdatabase/models/BirthdayReminderModel.js
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/birthdayScheduler.js
- commands/birthday_testreminder.js
Comment on lines
+48
to
49
| description: 'Timezone offset (e.g. +8, -5, +5.30, 10.00). Defaults to UTC.', | ||
| type: 3, |
There was a problem hiding this comment.
Minute-offset timezones are accepted, but downstream matching is hour-only.
Line 48 and Line 102 advertise/support minute offsets (e.g., +5.30), and Line 115 stores them in ISO form, but database/queries/userQueries.js (Lines 128-133) matches birthdays with strftime('%m-%dT%H', birthdays) (no minutes). Users in half/quarter-hour offsets can be notified at the wrong minute.
Consider either (a) rejecting non-00 minutes here for now, or (b) updating downstream query/scheduler matching to include minutes end-to-end.
Also applies to: 99-103, 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/birthday_set.js` around lines 48 - 49, The command accepts
minute-offset timezones (e.g., +5.30) but downstream matching in
database/queries/userQueries.js uses strftime('%m-%dT%H', birthdays)
(hour-only), so users with non-:00 minute offsets get wrong notifications;
either reject minute offsets at input or make matching minute-accurate. Fix by
updating commands/birthday_set.js timezone argument validation (the timezone
option and the code that builds the ISO datetime stored) to reject or normalize
any timezone strings with non-00 minutes (e.g., allow only offsets whose minutes
are 00) and return a clear validation error to the user, or alternatively update
the matching logic in database/queries/userQueries.js to use
strftime('%m-%dT%H:%M', birthdays) (and corresponding scheduler/job logic) so
minutes are matched end-to-end; choose one approach and make the change
consistently where timezone ISO is constructed and where strftime is used.
This was referenced Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
blah blah blah you can now be notified about someones birthday in advance
Summary by CodeRabbit
New Features
Improvements
Chores