feat: email reminders for follow-up dates via Brevo - #26
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds hourly follow-up reminder processing with Brevo email delivery, persisted reminder timestamps, dependency-injection wiring, shutdown-safe scheduling, database support, tests, and a follow-up reminder notice in the web application view. ChangesFollow-up reminders
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant ReminderUseCase
participant ApplicationRepository
participant EmailService
Scheduler->>ReminderUseCase: execute()
ReminderUseCase->>ApplicationRepository: findDueForReminder()
ReminderUseCase->>EmailService: sendFollowUpReminder()
ReminderUseCase->>ApplicationRepository: updateReminderSentAt()
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Adds a background job that sends follow-up reminder emails 24 h before a scheduled follow-up date using the Brevo transactional email API. - reminderSentAt DateTime? column on JobApplication (Prisma + migration) - IEmailService port + BrevoEmailService (raw fetch, no SDK) - findDueForReminder() + updateReminderSentAt() on IApplicationRepository implemented in PrismaApplicationRepository and delegated in CachedApplicationRepository - SendFollowUpRemindersUseCase: queries due apps, looks up user email, sends, marks sent - reminders.plugin.ts: Fastify plugin with 1-hour setInterval, registered in app.ts - container.ts: emailService (SINGLETON) + sendFollowUpRemindersUseCase (TRANSIENT) - env.example: BREVO_API_KEY, FROM_EMAIL, FROM_NAME - Unit tests: sends email, skips missing user, continues past individual failures - Web: "Email reminder will be sent 24 h before this date" note on detail page
6b356a3 to
7e5a868
Compare
![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
🧹 Nitpick comments (2)
apps/api/src/use-cases/reminders/SendFollowUpRemindersUseCase.ts (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog errors for failed email deliveries.
While correctly catching the error to prevent the loop from aborting, completely swallowing it reduces observability. Consider logging the error so that delivery failures can be tracked and debugged.
📝 Proposed refactor
- } catch { - // continue — one failure shouldn't block the rest + } catch (error) { + console.error(`Failed to send reminder for application ${app.id}:`, error); }🤖 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/api/src/use-cases/reminders/SendFollowUpRemindersUseCase.ts` around lines 27 - 29, Update the catch block in SendFollowUpRemindersUseCase to log each failed email delivery with the caught error and relevant delivery context, while preserving the existing behavior of continuing to process remaining reminders.apps/api/src/infrastructure/email/BrevoEmailService.ts (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant status code check.
The
response.okproperty evaluates totruefor any status code in the 200-299 range (including 201). If!response.okis true,response.statusis guaranteed not to be 201. The extra check is redundant.🧹 Proposed simplification
- if (!response.ok && response.status !== 201) { + if (!response.ok) { const body = await response.text(); throw new Error(`Brevo API error ${response.status}: ${body}`); }🤖 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/api/src/infrastructure/email/BrevoEmailService.ts` around lines 39 - 42, In the response validation within the Brevo email request flow, simplify the condition to rely solely on response.ok and remove the redundant response.status !== 201 check. Preserve the existing error body retrieval and Brevo API error construction for non-OK responses.
🤖 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/api/.env.example`:
- Around line 26-27: Update the FROM_NAME environment variable in the example
configuration to quote its space-containing value, while leaving FROM_EMAIL
unchanged.
In `@apps/api/src/http/plugins/reminders.plugin.ts`:
- Around line 7-14: Update the catch block in the run function to log the caught
error through Fastify’s built-in logger, while preserving the existing behavior
of preventing background reminder failures from crashing the server.
In `@apps/api/src/infrastructure/db/repositories/CachedApplicationRepository.ts`:
- Around line 73-81: Update CachedApplicationRepository.findDueForReminder to
populate the userIdByAppId mapping for every returned application, then update
updateReminderSentAt to use that mapping to invalidate the corresponding
apps:list:${userId}: cache entry in addition to the existing by-id cache
deletion; remove or safely handle the mapping entry after invalidation as
appropriate.
In `@apps/web/src/routes/_authenticated/applications/`$applicationId/index.tsx:
- Around line 192-194: Update the follow-up reminder notice near the
`followUpAt` display to render only when the follow-up date is in the future,
while preserving the existing non-null check. Replace the “24 h before” wording
with language indicating the email reminder is sent during the 24 hours before
the date, consistent with the hourly scheduler behavior.
---
Nitpick comments:
In `@apps/api/src/infrastructure/email/BrevoEmailService.ts`:
- Around line 39-42: In the response validation within the Brevo email request
flow, simplify the condition to rely solely on response.ok and remove the
redundant response.status !== 201 check. Preserve the existing error body
retrieval and Brevo API error construction for non-OK responses.
In `@apps/api/src/use-cases/reminders/SendFollowUpRemindersUseCase.ts`:
- Around line 27-29: Update the catch block in SendFollowUpRemindersUseCase to
log each failed email delivery with the caught error and relevant delivery
context, while preserving the existing behavior of continuing to process
remaining reminders.
🪄 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: bd06653a-199e-4bf3-bdcb-07b4f487d09c
📒 Files selected for processing (17)
apps/api/.env.exampleapps/api/prisma/migrations/20260721000001_add_reminder_sent_at/migration.sqlapps/api/prisma/schema.prismaapps/api/src/__tests__/application/reminders/SendFollowUpRemindersUseCase.test.tsapps/api/src/__tests__/helpers/createTestDb.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/app.tsapps/api/src/domain/application/Application.tsapps/api/src/http/container.tsapps/api/src/http/plugins/reminders.plugin.tsapps/api/src/infrastructure/db/repositories/CachedApplicationRepository.tsapps/api/src/infrastructure/db/repositories/PrismaApplicationRepository.tsapps/api/src/infrastructure/email/BrevoEmailService.tsapps/api/src/use-cases/ports/IApplicationRepository.tsapps/api/src/use-cases/ports/IEmailService.tsapps/api/src/use-cases/reminders/SendFollowUpRemindersUseCase.tsapps/web/src/routes/_authenticated/applications/$applicationId/index.tsx
| FROM_EMAIL=noreply@yourdomain.com | ||
| FROM_NAME=Job Finder |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Quote environment variables that contain spaces.
As flagged by the static analysis tools, variables containing spaces should be enclosed in quotes to ensure compatibility across different environment variable parsers and deployment platforms.
🛠 Proposed fix
FROM_EMAIL=noreply@yourdomain.com
-FROM_NAME=Job Finder
+FROM_NAME="Job Finder"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM_EMAIL=noreply@yourdomain.com | |
| FROM_NAME=Job Finder | |
| FROM_EMAIL=noreply@yourdomain.com | |
| FROM_NAME="Job Finder" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 27-27: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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/api/.env.example` around lines 26 - 27, Update the FROM_NAME environment
variable in the example configuration to quote its space-containing value, while
leaving FROM_EMAIL unchanged.
Source: Linters/SAST tools
| const run = async () => { | ||
| try { | ||
| const { sendFollowUpRemindersUseCase } = fastify.diContainer.cradle; | ||
| await sendFollowUpRemindersUseCase.execute(); | ||
| } catch { | ||
| // swallow — don't crash the server | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log errors instead of silently swallowing them.
Silently catching and ignoring errors will make it very difficult to debug if the background reminder process starts failing (e.g., due to database connectivity issues or email service misconfigurations). Consider logging the error using Fastify's built-in logger.
💻 Proposed fix
const run = async () => {
try {
const { sendFollowUpRemindersUseCase } = fastify.diContainer.cradle;
await sendFollowUpRemindersUseCase.execute();
- } catch {
- // swallow — don't crash the server
+ } catch (error) {
+ fastify.log.error(error, 'Failed to execute follow-up reminders process');
}
};📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const run = async () => { | |
| try { | |
| const { sendFollowUpRemindersUseCase } = fastify.diContainer.cradle; | |
| await sendFollowUpRemindersUseCase.execute(); | |
| } catch { | |
| // swallow — don't crash the server | |
| } | |
| }; | |
| const run = async () => { | |
| try { | |
| const { sendFollowUpRemindersUseCase } = fastify.diContainer.cradle; | |
| await sendFollowUpRemindersUseCase.execute(); | |
| } catch (error) { | |
| fastify.log.error(error, 'Failed to execute follow-up reminders process'); | |
| } | |
| }; |
🤖 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/api/src/http/plugins/reminders.plugin.ts` around lines 7 - 14, Update
the catch block in the run function to log the caught error through Fastify’s
built-in logger, while preserving the existing behavior of preventing background
reminder failures from crashing the server.
|
|
||
| async findDueForReminder(): Promise<Application[]> { | ||
| return this.inner.findDueForReminder(); | ||
| } | ||
|
|
||
| async updateReminderSentAt(id: string, sentAt: Date): Promise<void> { | ||
| await this.inner.updateReminderSentAt(id, sentAt); | ||
| this.cache.delete(`apps:byId:${id}`); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate the list cache to prevent stale application data.
When a reminder is recorded, the Application object is modified. Failing to invalidate the list cache (apps:list:${userId}:) will result in users seeing stale reminder statuses when viewing their applications.
To enable updateReminderSentAt to clear the correct list cache, findDueForReminder must also populate the userIdByAppId map.
♻️ Proposed fix to keep the cache consistent
- async findDueForReminder(): Promise<Application[]> {
- return this.inner.findDueForReminder();
- }
-
- async updateReminderSentAt(id: string, sentAt: Date): Promise<void> {
- await this.inner.updateReminderSentAt(id, sentAt);
- this.cache.delete(`apps:byId:${id}`);
- }
+ async findDueForReminder(): Promise<Application[]> {
+ const result = await this.inner.findDueForReminder();
+ for (const app of result) {
+ this.userIdByAppId.set(app.id, app.userId);
+ }
+ return result;
+ }
+
+ async updateReminderSentAt(id: string, sentAt: Date): Promise<void> {
+ await this.inner.updateReminderSentAt(id, sentAt);
+ this.cache.delete(`apps:byId:${id}`);
+ const userId = this.userIdByAppId.get(id);
+ if (userId) {
+ this.cache.deleteByPrefix(`apps:list:${userId}:`);
+ }
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async findDueForReminder(): Promise<Application[]> { | |
| return this.inner.findDueForReminder(); | |
| } | |
| async updateReminderSentAt(id: string, sentAt: Date): Promise<void> { | |
| await this.inner.updateReminderSentAt(id, sentAt); | |
| this.cache.delete(`apps:byId:${id}`); | |
| } | |
| async findDueForReminder(): Promise<Application[]> { | |
| const result = await this.inner.findDueForReminder(); | |
| for (const app of result) { | |
| this.userIdByAppId.set(app.id, app.userId); | |
| } | |
| return result; | |
| } | |
| async updateReminderSentAt(id: string, sentAt: Date): Promise<void> { | |
| await this.inner.updateReminderSentAt(id, sentAt); | |
| this.cache.delete(`apps:byId:${id}`); | |
| const userId = this.userIdByAppId.get(id); | |
| if (userId) { | |
| this.cache.deleteByPrefix(`apps:list:${userId}:`); | |
| } | |
| } |
🤖 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/api/src/infrastructure/db/repositories/CachedApplicationRepository.ts`
around lines 73 - 81, Update CachedApplicationRepository.findDueForReminder to
populate the userIdByAppId mapping for every returned application, then update
updateReminderSentAt to use that mapping to invalidate the corresponding
apps:list:${userId}: cache entry in addition to the existing by-id cache
deletion; remove or safely handle the mapping entry after invalidation as
appropriate.
| <p className="text-xs text-gray-400 mt-0.5"> | ||
| Email reminder will be sent 24 h before this date. | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid promising a reminder for past follow-ups.
This notice is rendered for any non-null followUpAt, including dates already in the past, although findDueForReminder() only selects dates from now through the next 24 hours. Also, the hourly scheduler sends within that window rather than exactly 24 hours before. Hide the notice for past dates and use wording such as “An email reminder is sent during the 24 hours before this date.”
🤖 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/routes/_authenticated/applications/`$applicationId/index.tsx
around lines 192 - 194, Update the follow-up reminder notice near the
`followUpAt` display to render only when the follow-up date is in the future,
while preserving the existing non-null check. Replace the “24 h before” wording
with language indicating the email reminder is sent during the 24 hours before
the date, consistent with the hourly scheduler behavior.
1 participant
Summary
Test plan
Summary by CodeRabbit