@remindme bot support #1159
@remindme bot support #1159
Conversation
* hasNewNotes implementation * actually return notification component in ui * delete reminder and job on item delete * other goodies
…of post to satisfy prisma * update wording in form toast for remindme bot usage * update wording in the push notification sent
WalkthroughWalkthroughThe changes introduce a reminder functionality across various components and modules. Users can now set reminders on items, which will notify them at a scheduled time. This involves adding new resolver functions, type definitions, and database schema changes to support reminders. Additionally, notification handling is updated to include reminders, and UI components are modified to display reminder-related messages. Changes
Assessment against linked issues
Overall, the changes effectively implement the core reminder functionality as requested in the linked issue, but the cost mechanism for setting reminders is not included and needs to be addressed separately. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Out of diff range and nitpick comments (1)
api/resolvers/notifications.js (1)
307-314: Addition of theReminderquery looks good and aligns with the PR objectives.Consider adding an index on the
remindAtcolumn in theRemindertable to optimize query performance, especially as the data grows.
api/resolvers/item.js
Outdated
| await models.$queryRawUnsafe(`DELETE FROM pgboss.job WHERE name = 'reminder' AND data->>'itemId' = '${item.id}' AND data->>'userId' = '${me.id}' AND state <> 'completed';`) | ||
| await models.reminder.deleteMany({ | ||
| where: { | ||
| itemId: Number(item.id), | ||
| userId: Number(me.id), | ||
| remindAt: { | ||
| gt: new Date() | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
nit: making this transactional is probably a good idea
api/resolvers/item.js
Outdated
| await models.$queryRawUnsafe(` | ||
| INSERT INTO pgboss.job (name, data, startafter) | ||
| VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s');`) | ||
| // use a raw query instead of the model to reuse the built-in `now + interval` support instead of doing it via JS | ||
| await models.$queryRawUnsafe(` | ||
| INSERT INTO "Reminder" ("userId", "itemId", "remindAt") | ||
| VALUES (${me.id}, ${item.id}, now() + interval '${reminderCommand.number} ${reminderCommand.unit}s');`) |
api/resolvers/item.js
Outdated
| if (reminderCommand) { | ||
| await models.$queryRawUnsafe(` | ||
| INSERT INTO pgboss.job (name, data, startafter) | ||
| VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s');`) |
There was a problem hiding this comment.
We'll need to set an expiresin here. By default this job will be ignored after 15 minutes: https://github.com/timgit/pg-boss/blob/master/docs/readme.md#job-table
I noticed this when I was queuing territoryBilling jobs that startafter 1 month or 1 year.
There was a problem hiding this comment.
I looked at the documentation now, especially since you also mentioned this to me. But afaict, expireIn is only relevant for active jobs so this shouldn't affect billing jobs that start (and then are active) in a month or year?
That would also match my expectations, why would pgboss drop jobs that haven't even started yet?
See documentation for job states:
All jobs start out in the created state and become active when picked up for work. If job processing completes successfully, jobs will go to completed. If a job fails, it will typically enter the failed state. However, if a job has retry options configured, it will enter the retry state on failure instead and have a chance to re-enter active state. It's also possible for active jobs to become expired, which happens when job processing takes too long. Jobs can also enter cancelled state via cancel(id) or cancel([ids]).
Expiration options
- expireInSeconds, number
How many seconds a job may be in active state before it is failed because of expiration. Must be >=1[...]
Default: 15 minutes
So what you're describing sounds like a bug
There was a problem hiding this comment.
That's a good point. I think I borrowed this syntax from the delete bot job, which I don't think had it either? I could be wrong. But I agree, it does seem like a bug if you need to set expiration for a job that starts more than 15 minutes in the future. I don't know if I explicitly tested a reminder that far in the future so I can't say if it was an issue here.
api/resolvers/item.js
Outdated
| const deleteReminderAndJob = async ({ me, item, models }) => { | ||
| if (me?.id && me.id !== ANON_USER_ID) { | ||
| await models.$queryRawUnsafe(`DELETE FROM pgboss.job WHERE name = 'reminder' AND data->>'itemId' = '${item.id}' AND data->>'userId' = '${me.id}' AND state <> 'completed';`) | ||
| await models.reminder.deleteMany({ | ||
| where: { | ||
| itemId: Number(item.id), | ||
| userId: Number(me.id), | ||
| remindAt: { | ||
| gt: new Date() | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor deletion of reminder jobs to handle non-completed jobs.
Consider adding a comment to clarify why only non-completed jobs are deleted. This could help future maintainers understand the decision-making process behind this logic.
+ // Delete non-completed reminder jobs to avoid interfering with already triggered reminders
await models.$queryRawUnsafe(`DELETE FROM pgboss.job WHERE name = 'reminder' AND data->>'itemId' = '${item.id}' AND data->>'userId' = '${me.id}' AND state <> 'completed';`)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const deleteReminderAndJob = async ({ me, item, models }) => { | |
| if (me?.id && me.id !== ANON_USER_ID) { | |
| await models.$queryRawUnsafe(`DELETE FROM pgboss.job WHERE name = 'reminder' AND data->>'itemId' = '${item.id}' AND data->>'userId' = '${me.id}' AND state <> 'completed';`) | |
| await models.reminder.deleteMany({ | |
| where: { | |
| itemId: Number(item.id), | |
| userId: Number(me.id), | |
| remindAt: { | |
| gt: new Date() | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| const deleteReminderAndJob = async ({ me, item, models }) => { | |
| if (me?.id && me.id !== ANON_USER_ID) { | |
| // Delete non-completed reminder jobs to avoid interfering with already triggered reminders | |
| await models.$queryRawUnsafe(`DELETE FROM pgboss.job WHERE name = 'reminder' AND data->>'itemId' = '${item.id}' AND data->>'userId' = '${me.id}' AND state <> 'completed';`) | |
| await models.reminder.deleteMany({ | |
| where: { | |
| itemId: Number(item.id), | |
| userId: Number(me.id), | |
| remindAt: { | |
| gt: new Date() | |
| } | |
| } | |
| }) | |
| } | |
| } |
api/resolvers/item.js
Outdated
| if (reminderCommand) { | ||
| await models.$queryRawUnsafe(` | ||
| INSERT INTO pgboss.job (name, data, startafter) | ||
| VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s');`) |
There was a problem hiding this comment.
Set expiration for reminder jobs.
The reminder jobs need an expiresin setting to ensure they do not persist indefinitely in the queue if not executed within a reasonable timeframe.
+ // Set an expiration for the reminder job to ensure it does not persist indefinitely
VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s', 'expiresin' '15 minutes');Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s');`) | |
| // Set an expiration for the reminder job to ensure it does not persist indefinitely | |
| VALUES ('reminder', jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), now() + interval '${reminderCommand.number} ${reminderCommand.unit}s', 'expiresin' '15 minutes'); |
|
Awesome! Thank you for the feedback, I'll work on it as soon as I can |
|
FYI I've made the changes myself which I just need to test now. |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
api/resolvers/item.js (1)
1319-1324: Consider adding logging for reminder job deletions and creations.Adding logging could help in debugging issues related to reminder job management. This could be particularly useful for monitoring the system and understanding the flow of reminder setting and deletion.
| // clean up any pending reminders, if triggered on this item and haven't been executed | ||
| if (hasReminderCommand(old.text)) { | ||
| await deleteReminderAndJob({ me, item: old, models }) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling when deleting reminders.
The deleteReminderAndJob function does not currently include any error handling mechanisms. It is important to wrap the database operations in a try-catch block to handle potential errors gracefully.
- Location:
api/resolvers/item.js, lines wheredeleteReminderAndJobis defined and used.
Analysis chain
Ensure proper error handling when deleting reminders.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `deleteReminderAndJob` handles errors gracefully.
# Test: Search for error handling in the function implementation.
ast-grep --lang javascript --pattern $'function deleteReminderAndJob($_, $_) {
$$$
try {
$$$
} catch ($_) {
$$$
}
$$$
}'
Length of output: 139
Script:
#!/bin/bash
# Locate the definition of the `deleteReminderAndJob` function and display its implementation.
ast-grep --lang javascript --pattern 'function deleteReminderAndJob($_, $_) { $$$ }'
Length of output: 86
Script:
#!/bin/bash
# Search for the definition of the `deleteReminderAndJob` function across the codebase.
rg 'function deleteReminderAndJob'
Length of output: 36
Script:
#!/bin/bash
# Search for the definition of the `deleteReminderAndJob` function across the codebase.
rg 'deleteReminderAndJob'
Length of output: 269
Script:
#!/bin/bash
# Extract the implementation of the `deleteReminderAndJob` function from `api/resolvers/item.js`.
sed -n '/const deleteReminderAndJob = async ({ me, item, models }) => {/,/^}/p' api/resolvers/item.js
Length of output: 662
| INSERT INTO pgboss.job (name, data, startafter, expirein) | ||
| VALUES ( | ||
| 'deleteItem', | ||
| jsonb_build_object('id', ${item.id}), | ||
| now() + interval '${deleteCommand.number} ${deleteCommand.unit}s', | ||
| interval '${deleteCommand.number} ${deleteCommand.unit}s' + interval '1 minute')`) |
There was a problem hiding this comment.
Validate the deleteCommand structure before using it.
It's important to validate the deleteCommand object to ensure that it contains valid number and unit properties before constructing the SQL query. This can prevent potential SQL injection or logical errors.
+ if (!deleteCommand || typeof deleteCommand.number !== 'number' || typeof deleteCommand.unit !== 'string') {
+ throw new Error('Invalid delete command');
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| INSERT INTO pgboss.job (name, data, startafter, expirein) | |
| VALUES ( | |
| 'deleteItem', | |
| jsonb_build_object('id', ${item.id}), | |
| now() + interval '${deleteCommand.number} ${deleteCommand.unit}s', | |
| interval '${deleteCommand.number} ${deleteCommand.unit}s' + interval '1 minute')`) | |
| if (!deleteCommand || typeof deleteCommand.number !== 'number' || typeof deleteCommand.unit !== 'string') { | |
| throw new Error('Invalid delete command'); | |
| } | |
| INSERT INTO pgboss.job (name, data, startafter, expirein) | |
| VALUES ( | |
| 'deleteItem', | |
| jsonb_build_object('id', ${item.id}), | |
| now() + interval '${deleteCommand.number} ${deleteCommand.unit}s', | |
| interval '${deleteCommand.number} ${deleteCommand.unit}s' + interval '1 minute')`) |
| const createReminderAndJob = async ({ me, item, models }) => { | ||
| // disallow anon to use reminder | ||
| if (!me || me.id === ANON_USER_ID) { | ||
| return | ||
| } | ||
| const reminderCommand = getReminderCommand(item.text) | ||
| if (reminderCommand) { | ||
| await models.$transaction([ | ||
| models.$queryRawUnsafe(` | ||
| INSERT INTO pgboss.job (name, data, startafter, expirein) | ||
| VALUES ( | ||
| 'reminder', | ||
| jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), | ||
| now() + interval '${reminderCommand.number} ${reminderCommand.unit}s', | ||
| interval '${reminderCommand.number} ${reminderCommand.unit}s' + interval '1 minute')`), | ||
| // use a raw query instead of the model to reuse the built-in `now + interval` support instead of doing it via JS | ||
| models.$queryRawUnsafe(` | ||
| INSERT INTO "Reminder" ("userId", "itemId", "remindAt") | ||
| VALUES (${me.id}, ${item.id}, now() + interval '${reminderCommand.number} ${reminderCommand.unit}s')`) | ||
| ]) |
There was a problem hiding this comment.
Refactor the reminder creation to handle potential exceptions.
The current implementation does not handle exceptions that might occur during the database operations. Wrapping these operations in a try-catch block could improve reliability and error handling.
+ try {
await models.$transaction([
models.$queryRawUnsafe(`
INSERT INTO pgboss.job (name, data, startafter, expirein)
VALUES (
'reminder',
jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}),
now() + interval '${reminderCommand.number} ${reminderCommand.unit}s',
interval '${reminderCommand.number} ${reminderCommand.unit}s' + interval '1 minute')`),
models.$queryRawUnsafe(`
INSERT INTO "Reminder" ("userId", "itemId", "remindAt")
VALUES (${me.id}, ${item.id}, now() + interval '${reminderCommand.number} ${reminderCommand.unit}s')`)
])
+ } catch (error) {
+ console.error('Failed to create reminder', error);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const createReminderAndJob = async ({ me, item, models }) => { | |
| // disallow anon to use reminder | |
| if (!me || me.id === ANON_USER_ID) { | |
| return | |
| } | |
| const reminderCommand = getReminderCommand(item.text) | |
| if (reminderCommand) { | |
| await models.$transaction([ | |
| models.$queryRawUnsafe(` | |
| INSERT INTO pgboss.job (name, data, startafter, expirein) | |
| VALUES ( | |
| 'reminder', | |
| jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), | |
| now() + interval '${reminderCommand.number} ${reminderCommand.unit}s', | |
| interval '${reminderCommand.number} ${reminderCommand.unit}s' + interval '1 minute')`), | |
| // use a raw query instead of the model to reuse the built-in `now + interval` support instead of doing it via JS | |
| models.$queryRawUnsafe(` | |
| INSERT INTO "Reminder" ("userId", "itemId", "remindAt") | |
| VALUES (${me.id}, ${item.id}, now() + interval '${reminderCommand.number} ${reminderCommand.unit}s')`) | |
| ]) | |
| const createReminderAndJob = async ({ me, item, models }) => { | |
| // disallow anon to use reminder | |
| if (!me || me.id === ANON_USER_ID) { | |
| return | |
| } | |
| const reminderCommand = getReminderCommand(item.text) | |
| if (reminderCommand) { | |
| try { | |
| await models.$transaction([ | |
| models.$queryRawUnsafe(` | |
| INSERT INTO pgboss.job (name, data, startafter, expirein) | |
| VALUES ( | |
| 'reminder', | |
| jsonb_build_object('itemId', ${item.id}, 'userId', ${me.id}), | |
| now() + interval '${reminderCommand.number} ${reminderCommand.unit}s', | |
| interval '${reminderCommand.number} ${reminderCommand.unit}s' + interval '1 minute')`), | |
| // use a raw query instead of the model to reuse the built-in `now + interval` support instead of doing it via JS | |
| models.$queryRawUnsafe(` | |
| INSERT INTO "Reminder" ("userId", "itemId", "remindAt") | |
| VALUES (${me.id}, ${item.id}, now() + interval '${reminderCommand.number} ${reminderCommand.unit}s')`) | |
| ]) | |
| } catch (error) { | |
| console.error('Failed to create reminder', error); | |
| } | |
| } | |
| } |
Description
Closes #510
This PR adds support for a
@remindmebot, similar to the@deletebot.It uses the same directive pattern, meaning you mention
@remindmefollowed by a known syntax to enable the functionality. The pattern is as follows:@remindme in <num> <units>where
<num>is a positive integer with no separators, and<units>is one ofsecond, minute, hours, day, week, month, year, or their plural counterparts. Some examples:@remindme in 1 day@remindme in 2 minutes@remindme in 4 monthsWhen you include this in either a post or a comment, a reminder is scheduled and you are given feedback in the UI that tells you when you will be reminded of this item, localized to your browser's timezone/locale.
If you have a mention of
@remindmebut the reminder was not scheduled, a toast message is shown to help guide you to the correct syntax for using the bot.If you delete an item that has a
@remindmebot mention, the reminder is canceled (deleted). Note: if the reminder has already occurred, it is not deleted, since we can't "unremind" you. That means it will still show up in your notification history.If you edit an item that has a
@remindmebot mention, the existing reminder is canceled (deleted). If the updated text contains another@remindmebot mention, the new reminder is scheduled accordingly. Similar to deleting an item with a@remindmemention, if the reminder has already occurred before you edit the containing item, the reminder is not deleted, so it will still show up in your notification history.Reminders are sent via notifications in your notifications page, as well as via push notification, if you have that enabled. The reminder links you to the item in which you mentioned
@remindme.So, for example, if someone else makes a post and you want to be reminded of it, you would reply via comment to the post with
@remindme in 1 week. 1 week later, you will be notified of your comment, and therefore the parent item implicitly.Technical overview:
Remindermodel added to the DB. It has 3 interesting fields:itemIduserIdremindAt@remindmementions and extract the configuration, and create theReminderinstances, and also schedule thereminderjobs.reminder- scheduled via the item API resolver, and handled via a new worker modulehasNewNotesremindAtin the past) in the notifications listingScreenshots
Successful reminder scheduled toast
Reminders in notifications page
Additional Context
We could get away with removing
userIdfrom theRemindermodel, since with the implementation, theuserIdin theReminderis always the owner of the item corresponding toitemId, so we could just look up the user id when needed. I originally modeled it with bothuserIdanditemIdsince that seemed natural, but it might be overkill.This approach does future-proof us if we ever decide to allow you to be directly reminded of someone else's items, though. So maybe it's fine? Open to input here.
Checklist
Are your changes backwards compatible? Please answer below:
Yes. New DB model but it is backwards compatible.
Did you QA this? Could we deploy this straight to production? Please answer below:
Yes.
For frontend changes: Tested on mobile? Please answer below:
No new patterns implemented, so I didn't test it on mobile.
Did you introduce any new environment variables? If so, call them out explicitly here:
No.