Feat/implement scheduled email reminders#65
Feat/implement scheduled email reminders#65laryhills wants to merge 6 commits intoAURORALAOrg:mainfrom
Conversation
…d email notifications
…p user reminder job logs
WalkthroughIntroduces inactivity reminder feature: adds user timestamp fields, environment settings, cron-based scheduler, notification and email sending services, a reminder job, and integrates job initialization/shutdown into server. Auth controller and middleware now record login/activity. SMTP transporter settings updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Timer as Cron (node-cron)
participant Server
participant Scheduler as JobSchedulerService
participant Job as UserReminderJob
participant Notify as NotificationService
participant UserSvc as UserService
participant Mail as EmailNotifier
Server->>Scheduler: initializeJobs()
Scheduler->>Timer: schedule(CRON_SCHEDULE, job)
note over Scheduler,Timer: Scheduled in UTC
Timer-->>Scheduler: tick at schedule
Scheduler->>Job: checkInactiveUsers(maxRetries)
Job->>Notify: getReminderStats()
Notify->>UserSvc: countInactiveUsers(>=7d)
UserSvc-->>Notify: totalInactive
Notify-->>Job: stats
Job->>Notify: findReminderCandidates()
Notify->>UserSvc: findInactiveUsers(>=7d)
UserSvc-->>Notify: inactive users
Notify-->>Job: candidates (with types)
Job->>Mail: sendReminderEmails(reminders[], retries)
alt partial failures
Mail-->>Job: results (success/failed/errors)
else all success
Mail-->>Job: results (all success)
end
Job->>UserSvc: updateReminderSentBatch(userIds)
UserSvc-->>Job: ok
Job-->>Scheduler: JobResult (counts, errors, duration)
Scheduler-->>Server: log outcome
rect rgba(230,245,255,0.4)
note over Server,UserSvc: Separate flow on requests
end
sequenceDiagram
autonumber
participant Client
participant Auth as Auth Controller
participant MW as Auth Middleware
participant UserSvc as UserService
Client->>Auth: POST /login
Auth->>UserSvc: updateLastLogin(userId)
UserSvc-->>Auth: ok
Auth-->>Client: JWT
Client->>MW: Authenticated request
MW->>UserSvc: updateLastActivity(userId)
UserSvc-->>MW: ok
MW-->>Client: proceed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration 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: 10
🧹 Nitpick comments (5)
backend/.env.example (2)
34-34: Remove extra blank line.The linter detected an unnecessary blank line here.
-
35-41: Fix formatting issues and key ordering.The reminder configuration values are appropriate, but there are formatting issues that should be addressed based on linter feedback.
# ======================== # User Reminder CronJob Settings # ======================== -REMINDER_ENABLED=true CRON_SCHEDULE="0 2 * * *" +REMINDER_ENABLED=true REMINDER_MAX_RETRIES=2 REMINDER_RETRY_INTERVAL_HOURS=24 +The CRON schedule "0 2 * * *" (daily at 2 AM UTC) is well-chosen for off-peak execution as specified in the objectives.
backend/src/middlewares/authentication.ts (1)
28-29: Consider performance impact of synchronous activity tracking.Adding a database write operation to every authenticated request could impact response times and increase database load. Consider making this operation asynchronous or implementing a batching mechanism for high-traffic applications.
Consider this alternative approach for better performance:
- // Track user activity - await UserService.updateLastActivity(user.id); + // Track user activity asynchronously + UserService.updateLastActivity(user.id).catch(err => + console.error("Failed to track user activity:", err) + );Or implement a batching system to reduce database writes.
backend/src/services/jobScheduler.service.ts (1)
81-87: Improve job status checking reliabilityThe string comparison
job.getStatus() === "scheduled"is fragile and depends on internal implementation details of node-cron.Consider using the built-in method or storing state locally:
status.push({ name, - isRunning: job.getStatus() === "scheduled", + isRunning: job.running !== false, // node-cron sets running to false when stopped schedule: name === "userReminder" ? settings.reminders.cronSchedule : "unknown", });backend/src/jobs/userReminder.job.ts (1)
141-144: Health status should reflect actual job execution historyThe method always returns the current time as
lastRun, which doesn't reflect when the job actually last executed.Track actual execution times:
+ private static lastExecutionTime?: Date; + public static async checkInactiveUsers(maxRetries: number = 2): Promise<JobResult> { const startTime = Date.now(); + this.lastExecutionTime = new Date(); console.log("🔄 Starting user reminder job..."); // In getHealthStatus: return { isHealthy: true, - lastRun: new Date(), + lastRun: this.lastExecutionTime, nextRun: "Based on CRON schedule", };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/logs/error.logis excluded by!**/*.logbackend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
backend/.env.example(1 hunks)backend/package.json(4 hunks)backend/prisma/migrations/migration_lock.toml(1 hunks)backend/prisma/schema.prisma(1 hunks)backend/src/controllers/auth.controller.ts(1 hunks)backend/src/core/config/settings.ts(2 hunks)backend/src/db.ts(1 hunks)backend/src/jobs/userReminder.job.ts(1 hunks)backend/src/middlewares/authentication.ts(1 hunks)backend/src/server.ts(1 hunks)backend/src/services/jobScheduler.service.ts(1 hunks)backend/src/services/notification.service.ts(1 hunks)backend/src/services/user.service.ts(1 hunks)backend/src/utils/service/emailNotifier.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
backend/src/server.ts (1)
Learnt from: PoulavBhowmick03
PR: #58
File: backend/src/jobs/dailyStreak.ts:2-2
Timestamp: 2025-07-11T05:46:56.577Z
Learning: In the AURORALAOrg/aurora-backend project, Prisma is used by importing { PrismaClient } from "@prisma/client" and then instantiating it with const prisma = new PrismaClient(). This pattern is used consistently across service files like user.service.ts and question.service.ts.
backend/src/services/user.service.ts (1)
Learnt from: PoulavBhowmick03
PR: #58
File: backend/src/jobs/dailyStreak.ts:2-2
Timestamp: 2025-07-11T05:46:56.577Z
Learning: In the AURORALAOrg/aurora-backend project, Prisma is used by importing { PrismaClient } from "@prisma/client" and then instantiating it with const prisma = new PrismaClient(). This pattern is used consistently across service files like user.service.ts and question.service.ts.
🧬 Code Graph Analysis (1)
backend/src/services/user.service.ts (1)
backend/src/core/api/ApiError.ts (1)
InternalError(71-75)
🪛 dotenv-linter (3.3.0)
backend/.env.example
[warning] 34-34: [ExtraBlankLine] Extra blank line detected
[warning] 39-39: [UnorderedKey] The CRON_SCHEDULE key should go before the REMINDER_ENABLED key
[warning] 41-41: [EndingBlankLine] No blank line at the end of the file
🔇 Additional comments (11)
backend/src/db.ts (1)
23-23: LGTM! Minor grammatical improvement.The change from "error" to "errors" is a small but appropriate improvement since the error parameter could contain multiple error details.
backend/prisma/migrations/migration_lock.toml (1)
4-4: LGTM! Proper file ending.Adding a newline at the end of the file follows best practices and prevents potential issues with tools that expect files to end with newlines.
backend/prisma/schema.prisma (1)
39-41: LGTM! Well-designed schema additions for user activity tracking.The three new optional DateTime fields are perfectly aligned with the PR objectives:
lastLoginAt- tracks user login timeslastActivityAt- tracks general user activitylastReminderSent- prevents duplicate reminder emailsThe fields are appropriately optional and follow consistent naming conventions.
backend/package.json (2)
14-14: LGTM! Seed script addition is appropriate.The new seed script supports database initialization for the user activity tracking fields.
23-23: node-cron dependency versions verified
- Confirmed backend/package.json declares
• "node-cron": "^4.2.1" (latest stable: 4.2.1)
• "@types/node-cron": "^3.0.11" (latest stable: 3.0.11)- No known security advisories against these releases
Dependencies are up-to-date and safe to merge.
backend/src/core/config/settings.ts (2)
25-36: LGTM! Well-structured reminder configuration.The reminder configuration follows good practices with:
- Sensible defaults (2 AM daily schedule, 2 retries, 24-hour intervals)
- Proper Joi validation
- Clear descriptions
- Feature toggle with REMINDER_ENABLED
59-64: LGTM! Configuration mapping is consistent.The reminders section properly maps environment variables to the settings object with consistent naming.
backend/src/services/user.service.ts (4)
61-74: LGTM! Proper login tracking implementation.Updates both login and activity timestamps appropriately with proper error handling.
76-86: LGTM! Smart error handling for activity tracking.The decision not to throw errors for activity tracking is wise - it prevents authentication failures due to non-critical tracking issues.
88-125: LGTM! Well-designed inactive user query.The query logic correctly handles both scenarios:
- Users with
lastActivityAtolder than cutoff- Users with
null lastActivityAtbutcreatedAtolder than cutoffThe field selection is appropriate for reminder processing, and error handling is consistent.
127-137: LGTM! Standard reminder tracking update.Proper implementation for tracking when reminders were sent with consistent error handling.
|
Hello sir @laryhills. Thanks for your contribution. Please address every coderabbit comment/feedback please. |
|
I have resolved the comments |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/services/notification.service.ts (1)
75-77: Handle potential edge case with Infinity.When
user.lastReminderSentis null,daysSinceLastReminderis set toInfinity. While this works mathematically, it could be more explicit and readable.Consider using a more explicit approach:
- const daysSinceLastReminder = user.lastReminderSent - ? this.daysBetween(user.lastReminderSent, now) - : Infinity; + const daysSinceLastReminder = user.lastReminderSent + ? this.daysBetween(user.lastReminderSent, now) + : 999; // Large number to indicate no previous reminderbackend/src/services/user.service.ts (1)
169-183: Optimize batch update implementation.The current batch update uses individual
UPDATEstatements within a transaction. For better performance with large user sets, consider using a singleupdateManyoperation.public static async updateReminderSentBatch(userIds: string[]): Promise<void> { try { - await prisma.$transaction( - userIds.map(userId => - prisma.user.update({ - where: { id: userId }, - data: { lastReminderSent: new Date() } - }) - ) - ); + await prisma.user.updateMany({ + where: { + id: { + in: userIds + } + }, + data: { lastReminderSent: new Date() } + }); } catch (error) { console.error("Failed to batch update reminder sent:", error); throw new InternalError("Failed to batch update reminder sent"); } }This approach is more efficient for large batches and simpler to understand.
backend/src/utils/service/emailNotifier.ts (1)
112-112: Consider making retry parameters configurable.While the implementation is correct, hardcoded default values for
maxRetriesandretryDelayMsshould ideally come from configuration.Consider reading these values from environment variables or configuration:
- public static async sendReminderEmails(reminders: ReminderEmailData[], maxRetries: number = 2, retryDelayMs: number = 5000) { + public static async sendReminderEmails( + reminders: ReminderEmailData[], + maxRetries: number = parseInt(process.env.EMAIL_MAX_RETRIES || '2'), + retryDelayMs: number = parseInt(process.env.EMAIL_RETRY_DELAY_MS || '5000') + ) {This would allow for environment-specific tuning without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/controllers/auth.controller.ts(1 hunks)backend/src/jobs/userReminder.job.ts(1 hunks)backend/src/server.ts(1 hunks)backend/src/services/jobScheduler.service.ts(1 hunks)backend/src/services/notification.service.ts(1 hunks)backend/src/services/user.service.ts(1 hunks)backend/src/utils/service/emailNotifier.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/server.ts
- backend/src/controllers/auth.controller.ts
- backend/src/services/jobScheduler.service.ts
- backend/src/jobs/userReminder.job.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in the auroralaorg/aurora-backend project, prisma is used by importing { prismaclient } from "@prism...
Learnt from: PoulavBhowmick03
PR: AURORALAOrg/aurora-backend#58
File: backend/src/jobs/dailyStreak.ts:2-2
Timestamp: 2025-07-11T05:46:56.577Z
Learning: In the AURORALAOrg/aurora-backend project, Prisma is used by importing { PrismaClient } from "@prisma/client" and then instantiating it with `const prisma = new PrismaClient()`. This pattern is used consistently across service files like user.service.ts and question.service.ts.
Applied to files:
backend/src/services/user.service.ts
🔇 Additional comments (9)
backend/src/services/notification.service.ts (3)
23-26: Good fix - Past issue addressed correctly.The
daysBetweencalculation now usesMath.floorinstead ofMath.ceil, ensuring only complete days are counted. This addresses the previous review concern about partial days being rounded up incorrectly.
107-109: Excellent optimization - Redundant query eliminated.The redundant database query has been properly addressed by using
UserService.countInactiveUsers(7)instead of callingfindInactiveUserstwice. This eliminates duplicate database queries while maintaining the same functionality.
49-60: Review the reminder interval logic for consistency.The
shouldSendRemindermethod defines minimum intervals that may not align with business expectations:
7-dayreminders can be sent every 7 days14-dayreminders can be sent every 7 days30-dayreminders can be sent every 14 days60-dayreminders can be sent every 30 daysThis means a user could receive a
14-dayreminder, then another14-dayreminder 7 days later (at 21 days of inactivity), which seems excessive.Consider if these intervals align with your business requirements. The current logic allows:
- A user inactive for 14 days gets a
14-dayreminder- 7 days later (21 days inactive), they get another
14-dayreminder- 9 days later (30 days inactive), they get a
30-dayreminderWould it be more appropriate to ensure each reminder type is sent only once per inactivity period?
backend/src/services/user.service.ts (3)
61-74: LGTM! Well-implemented login tracking.The
updateLastLoginmethod correctly updates bothlastLoginAtandlastActivityAttimestamps, which makes sense since login is a form of activity. Error handling is appropriate with logging and throwingInternalError.
76-86: Good error handling strategy for activity tracking.The
updateLastActivitymethod wisely doesn't throw errors to avoid breaking user requests if activity tracking fails. This is a good design decision since activity tracking is supplementary functionality.
88-125: Verify inactive user query logicThe query correctly targets inactive users by combining:
lastActivityAt ≤ cutoffDate- OR
lastActivityAtis null ANDcreatedAt ≤ cutoffDate- Only
ACTIVEusers withisEmailVerified: trueare includedNo other similar queries were found in the codebase, so please manually confirm the following edge cases:
- Cutoff boundary: users with timestamps exactly equal to
cutoffDate- Users created within the cutoff window who never had activity (should be excluded)
- Time zone consistency between application server and database
- Behavior when
lastActivityAtorcreatedAtis invalid or missingbackend/src/utils/service/emailNotifier.ts (3)
11-23: Excellent security improvement - URL validation implemented.The
getValidatedAppUrlmethod properly addresses the previous security concern by validating the environment variable and ensuring it's a valid URL format before use in email templates.
33-97: Well-structured email content generation.The
getReminderEmailContentmethod provides appropriate messaging for different reminder types with escalating urgency. The email content is professional and engaging.
112-152: Improved retry implementation with proper delay.The retry logic has been significantly improved from the previous version:
- Proper delay between retries using
setTimeout- Tracking of successful emails in
successfulEmailsarray- Changed parameter from
retryIntervalHourstoretryDelayMsfor more precise controlThis addresses the previous review concerns effectively.
|
Hello @laryhills. Thanks for addressing the coderabbit suggestions. Could you please explain me how to check if everything that you did is working successfully? |
|
@Gerson2102 |
|
Hello @laryhills. I just run /root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:859
return new TSError(diagnosticText, diagnosticCodes, diagnostics);
^
TSError: ⨯ Unable to compile TypeScript:
src/services/user.service.ts:66:11 - error TS2353: Object literal may only specify known properties, and 'lastLoginAt' does not exist in type '(Without<UserUpdateInput, UserUncheckedUpdateInput> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput)'.
66 lastLoginAt: new Date(),
~~~~~~~~~~~
node_modules/.prisma/client/index.d.ts:2466:5
2466 data: XOR<UserUpdateInput, UserUncheckedUpdateInput>
~~~~
The expected type comes from property 'data' which is declared here on type '{ select?: UserSelect<DefaultArgs> | null | undefined; omit?: UserOmit<DefaultArgs> | null | undefined; include?: UserInclude<DefaultArgs> | null | undefined; data: (Without<...> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput); where: UserWhereUniqueInput; }'
src/services/user.service.ts:80:17 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type '(Without<UserUpdateInput, UserUncheckedUpdateInput> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput)'.
80 data: { lastActivityAt: new Date() },
~~~~~~~~~~~~~~
node_modules/.prisma/client/index.d.ts:2466:5
2466 data: XOR<UserUpdateInput, UserUncheckedUpdateInput>
~~~~
The expected type comes from property 'data' which is declared here on type '{ select?: UserSelect<DefaultArgs> | null | undefined; omit?: UserOmit<DefaultArgs> | null | undefined; include?: UserInclude<DefaultArgs> | null | undefined; data: (Without<...> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput); where: UserWhereUniqueInput; }'
src/services/user.service.ts:99:15 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type 'UserWhereInput'.
99 lastActivityAt: {
~~~~~~~~~~~~~~
src/services/user.service.ts:104:15 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type 'UserWhereInput'.
104 lastActivityAt: null,
~~~~~~~~~~~~~~
src/services/user.service.ts:116:11 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type 'UserSelect<DefaultArgs>'.
116 lastActivityAt: true,
~~~~~~~~~~~~~~
src/services/user.service.ts:138:15 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type 'UserWhereInput'.
138 lastActivityAt: {
~~~~~~~~~~~~~~
src/services/user.service.ts:143:15 - error TS2353: Object literal may only specify known properties, and 'lastActivityAt' does not exist in type 'UserWhereInput'.
143 lastActivityAt: null,
~~~~~~~~~~~~~~
src/services/user.service.ts:161:17 - error TS2353: Object literal may only specify known properties, and 'lastReminderSent' does not exist in type '(Without<UserUpdateInput, UserUncheckedUpdateInput> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput)'.
161 data: { lastReminderSent: new Date() },
~~~~~~~~~~~~~~~~
node_modules/.prisma/client/index.d.ts:2466:5
2466 data: XOR<UserUpdateInput, UserUncheckedUpdateInput>
~~~~
The expected type comes from property 'data' which is declared here on type '{ select?: UserSelect<DefaultArgs> | null | undefined; omit?: UserOmit<DefaultArgs> | null | undefined; include?: UserInclude<DefaultArgs> | null | undefined; data: (Without<...> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput); where: UserWhereUniqueInput; }'
src/services/user.service.ts:175:21 - error TS2353: Object literal may only specify known properties, and 'lastReminderSent' does not exist in type '(Without<UserUpdateInput, UserUncheckedUpdateInput> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput)'.
175 data: { lastReminderSent: new Date() }
~~~~~~~~~~~~~~~~
node_modules/.prisma/client/index.d.ts:2466:5
2466 data: XOR<UserUpdateInput, UserUncheckedUpdateInput>
~~~~
The expected type comes from property 'data' which is declared here on type '{ select?: UserSelect<DefaultArgs> | null | undefined; omit?: UserOmit<DefaultArgs> | null | undefined; include?: UserInclude<DefaultArgs> | null | undefined; data: (Without<...> & UserUncheckedUpdateInput) | (Without<...> & UserUpdateInput); where: UserWhereUniqueInput; }'
at createTSError (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:859:12)
at reportTSError (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:863:19)
at getOutput (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:1077:36)
at Object.compile (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:1433:41)
at Module.m._compile (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:1617:30)
at node:internal/modules/cjs/loader:1699:10
at Object.require.extensions.<computed> [as .ts] (/root/AURORA-Backend/backend/node_modules/ts-node/src/index.ts:1621:12)
at Module.load (node:internal/modules/cjs/loader:1313:32)
at Function._load (node:internal/modules/cjs/loader:1123:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14) {
diagnosticCodes: [
2353, 2353, 2353,
2353, 2353, 2353,
2353, 2353, 2353
]
}
|
|
Could you help me on telling me the list of commands that I should run to have everything setted up to see if everything is working coorectly? I ran |
|
@Gerson2102 |
|
hey @laryhills all goood this is my .env I forgot something? i migrated the db |
|
for better communication , feel free to contact me by telegram group Chris742001, thanks |
|
@ChrisFernandezVivas |
|
Hello @laryhills please rebase sir. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
backend/src/middlewares/authentication.ts (2)
29-31: Make activity tracking non-blocking and throttle updates to reduce DB load and request latencyUpdating lastActivityAt on every authorized request as an awaited call will amplify writes and add avoidable latency. Fire-and-forget and only update if stale (e.g., older than 5 minutes). Also, log failures at warn level without failing the request.
Apply within this hunk:
- // Track user activity - await UserService.updateLastActivity(user.id); + // Track user activity (non-blocking + throttle to once per 5 minutes) + const last = user.lastActivityAt ? new Date(user.lastActivityAt).getTime() : 0; + const now = Date.now(); + if (now - last > 5 * 60 * 1000) { + void UserService.updateLastActivity(user.id).catch((err) => + logger.warn("lastActivityAt update failed", { + userId: user.id, + error: err instanceof Error ? err.message : String(err), + }) + ); + }Additionally (outside this hunk), consider typing req.user and aligning locals naming:
// at top of file import type { User } from "@prisma/client"; // below: interface interface AuthenticatedRequest extends Request { user?: User; } // where you set locals res.locals.user = user; // prefer `user` over `account` for consistency
32-34: Unify locals naming for clarityres.locals.account mixes terminology with req.user. Prefer res.locals.user for consistency across the codebase and templates. This reduces mental overhead in handlers/views.
backend/src/services/user.service.ts (4)
80-90: Don’t return the updated user and avoid console.error; keep it lightweightSince callers shouldn’t depend on this value (and you already swallow errors), return void to make intent explicit and cut payload. Replace console.error with central logger for uniform observability.
- public static async updateLastActivity(userId: string) { + public static async updateLastActivity(userId: string): Promise<void> { try { - return await prisma.user.update({ + await prisma.user.update({ where: { id: userId }, data: { lastActivityAt: new Date() }, }); } catch (error) { - console.error("Failed to update last activity:", error); + // logger.warn({ err: error, userId }, "Failed to update last activity"); + console.warn("Failed to update last activity:", error); // Don't throw error for activity tracking to avoid breaking requests } }Note: With this change, ensure callers do not rely on a return value (they shouldn’t).
161-171: LGTM; consider reusing a single Date and central loggerThe method is correct. Minor polish: use a single now and central logger to match the rest of the service.
- public static async updateReminderSent(userId: string) { + public static async updateReminderSent(userId: string) { try { - return await prisma.user.update({ + const now = new Date(); + return await prisma.user.update({ where: { id: userId }, - data: { lastReminderSent: new Date() }, + data: { lastReminderSent: now }, }); } catch (error) { - console.error("Failed to update reminder sent:", error); + // logger.error({ err: error, userId }, "Failed to update reminder sent"); + console.error("Failed to update reminder sent:", error); throw new InternalError("Failed to update reminder sent"); } }
173-187: Batch update: use updateMany with a single timestamp and guard empty inputA single updateMany is more efficient than N updates in a transaction. Also avoid a no-op transaction on empty arrays and reuse a single timestamp.
- public static async updateReminderSentBatch(userIds: string[]): Promise<void> { + public static async updateReminderSentBatch(userIds: string[]): Promise<void> { try { - await prisma.$transaction( - userIds.map(userId => - prisma.user.update({ - where: { id: userId }, - data: { lastReminderSent: new Date() } - }) - ) - ); + if (!userIds.length) return; + const now = new Date(); + await prisma.user.updateMany({ + where: { id: { in: userIds } }, + data: { lastReminderSent: now }, + }); } catch (error) { console.error("Failed to batch update reminder sent:", error); throw new InternalError("Failed to batch update reminder sent"); } }
65-78: All Prisma schema fields verifiedThe Prisma schema at
backend/prisma/schema.prismaalready includeslastLoginAt,lastActivityAt, andlastReminderSent, so you won’t run into missing-field errors at compile time or during migration.Optional refactoring suggestions for
backend/src/services/user.service.ts:
- Allocate the current timestamp once to ensure both fields use the exact same value.
- Swap
console.errorfor your centrallogger.errorto standardize logging across services.Suggested diff:
--- a/backend/src/services/user.service.ts +++ b/backend/src/services/user.service.ts @@ public static async updateLastLogin(userId: string) { - return await prisma.user.update({ + const now = new Date(); + return await prisma.user.update({ where: { id: userId }, data: { - lastLoginAt: new Date(), - lastActivityAt: new Date() + lastLoginAt: now, + lastActivityAt: now }, }); @@ public static async updateLastLogin(userId: string) { - console.error("Failed to update last login:", error); + // TODO: replace console.error with your central logger import + // logger.error({ error, userId }, "Failed to update last login"); + console.error("Failed to update last login:", error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
backend/package.json(4 hunks)backend/src/controllers/auth.controller.ts(1 hunks)backend/src/middlewares/authentication.ts(1 hunks)backend/src/services/user.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/controllers/auth.controller.ts
- backend/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/services/user.service.ts (1)
backend/src/core/api/ApiError.ts (1)
InternalError(71-75)
🔇 Additional comments (1)
backend/src/services/user.service.ts (1)
92-129: Sanity checks complete – schema, cooldown logic, and cron wiring are all correctAll requested verifications passed:
- Prisma schema includes
lastLoginAt,lastActivityAt, andlastReminderSenton theUsermodel (backend/prisma/schema.prisma lines 39–41).NotificationServiceapplies a cooldown viashouldSendReminder(daysSinceLastReminder, reminderType), with minimum intervals mapped correctly for each reminder type (backend/src/services/notification.service.ts lines 49–57).- The job scheduler reads
CRON_SCHEDULE(default"0 2 * * *") from settings and registers it withcron.schedule(cronSchedule, …)injobScheduler.service.ts(backend/src/services/jobScheduler.service.ts lines 35–38).No further changes are needed here.
| public static async findInactiveUsers(days: number) { | ||
| const cutoffDate = new Date(); | ||
| cutoffDate.setDate(cutoffDate.getDate() - days); | ||
|
|
||
| try { | ||
| return await prisma.user.findMany({ | ||
| where: { | ||
| status: Status.ACTIVE, | ||
| isEmailVerified: true, | ||
| OR: [ | ||
| { | ||
| lastActivityAt: { | ||
| lte: cutoffDate, | ||
| }, | ||
| }, | ||
| { | ||
| lastActivityAt: null, | ||
| createdAt: { | ||
| lte: cutoffDate, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| select: { | ||
| id: true, | ||
| email: true, | ||
| firstName: true, | ||
| lastName: true, | ||
| lastActivityAt: true, | ||
| lastReminderSent: true, | ||
| createdAt: true, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to find inactive users:", error); | ||
| throw new InternalError("Failed to find inactive users"); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Filter out recently reminded users at the DB layer to prevent duplicate emails
Unless NotificationService already enforces a cooldown using lastReminderSent, push that predicate into the query so candidates are correct by construction. Default a 24-hour cooldown.
Apply this refactor in-place:
- public static async findInactiveUsers(days: number) {
+ public static async findInactiveUsers(days: number, minHoursBetweenReminders = 24) {
const cutoffDate = new Date();
cutoffDate.setDate(cutoffDate.getDate() - days);
+ const remindCutoff = new Date(Date.now() - minHoursBetweenReminders * 60 * 60 * 1000);
try {
return await prisma.user.findMany({
where: {
status: Status.ACTIVE,
isEmailVerified: true,
- OR: [
- {
- lastActivityAt: {
- lte: cutoffDate,
- },
- },
- {
- lastActivityAt: null,
- createdAt: {
- lte: cutoffDate,
- },
- },
- ],
+ AND: [
+ {
+ OR: [
+ { lastActivityAt: { lte: cutoffDate } },
+ { lastActivityAt: null, createdAt: { lte: cutoffDate } },
+ ],
+ },
+ {
+ OR: [
+ { lastReminderSent: null },
+ { lastReminderSent: { lt: remindCutoff } },
+ ],
+ },
+ ],
},
select: {
id: true,
email: true,
firstName: true,
lastName: true,
lastActivityAt: true,
lastReminderSent: true,
createdAt: true,
},
});Operational advice: add DB indexes to keep this fast under load (Prisma schema indices example):
@@index([status, isEmailVerified, lastActivityAt])
@@index([status, isEmailVerified, createdAt])
@@index([status, isEmailVerified, lastReminderSent])If NotificationService already applies cooldown logic, keep that and consider narrowing the DB result set anyway for efficiency.
🏁 Script executed:
#!/bin/bash
set -e
# 1. Locate NotificationService implementation
echo "🔍 Searching for NotificationService class..."
rg -n "class\s\+NotificationService" -g "backend/src/services/**/*.ts" || true
# 2. Search for lastReminderSent references in NotificationService
echo "🔍 Checking for lastReminderSent usage in NotificationService..."
rg -n "lastReminderSent" -g "backend/src/services/notification.service.ts" || true
# 3. Search for findInactiveUsers occurrences to see how it's consumed
echo "🔍 Finding usages of findInactiveUsers..."
rg -n "findInactiveUsers" -g "backend/src/services/**/*.ts" || true
# 4. Locate the Prisma schema file
echo "🔍 Locating schema.prisma..."
SCHEMA_FILE=$(find . -maxdepth 2 -type f -name "schema.prisma" | head -n1)
if [[ -z "$SCHEMA_FILE" ]]; then
echo "schema.prisma not found in root or prisma/ directory"
exit 1
else
echo "Found schema: $SCHEMA_FILE"
fi
# 5. Extract the User model definition from the schema
echo "🔍 Extracting model User from $SCHEMA_FILE..."
sed -n '/model User {/,/}/p' "$SCHEMA_FILE" || trueLength of output: 1341
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Showing NotificationService (lines 1-120)"
sed -n '1,120p' backend/src/services/notification.service.ts || true
echo -e "\n🔍 Searching for cooldown logic in NotificationService"
rg -nP --context 3 "daysSinceLastReminder" -g backend/src/services/notification.service.ts || true
echo -e "\n🔍 Finding all .prisma files"
find . -type f -name "*.prisma"Length of output: 7388
Add DB-layer cooldown filtering to findInactiveUsers
Currently, findInactiveUsers returns all users inactive for the given period, and the NotificationService applies a reminder cooldown in-memory. Pushing at least a 24-hour cooldown into the Prisma query both prevents duplicate sends at the source and shrinks the result set sent over the wire.
• In backend/src/services/user.service.ts, update findInactiveUsers to accept an optional minHoursBetweenReminders (defaulting to 24), compute remindCutoff, and add an AND clause that only returns users whose lastReminderSent is null or older than remindCutoff.
• In backend/prisma/schema.prisma, add an index on [status, isEmailVerified, lastReminderSent] to keep the new filter performant.
--- a/backend/src/services/user.service.ts
+++ b/backend/src/services/user.service.ts
@@ public static async findInactiveUsers(days: number) {
- public static async findInactiveUsers(days: number) {
+ public static async findInactiveUsers(
+ days: number,
+ minHoursBetweenReminders = 24,
+ ) {
const cutoffDate = new Date();
cutoffDate.setDate(cutoffDate.getDate() - days);
+ // Only re-remind users after at least minHoursBetweenReminders have passed
+ const remindCutoff = new Date(Date.now() - minHoursBetweenReminders * 60 * 60 * 1000);
try {
return await prisma.user.findMany({
where: {
status: Status.ACTIVE,
isEmailVerified: true,
- OR: [
- { lastActivityAt: { lte: cutoffDate } },
- { lastActivityAt: null, createdAt: { lte: cutoffDate } },
- ],
+ AND: [
+ {
+ OR: [
+ { lastActivityAt: { lte: cutoffDate } },
+ { lastActivityAt: null, createdAt: { lte: cutoffDate } },
+ ],
+ },
+ {
+ OR: [
+ { lastReminderSent: null },
+ { lastReminderSent: { lt: remindCutoff } },
+ ],
+ },
+ ],
},
select: {
id: true,
email: true,
firstName: true,
lastName: true,
lastActivityAt: true,
lastReminderSent: true,
createdAt: true,
},
});
} catch (error) {
console.error("Failed to find inactive users:", error);
throw new InternalError("Failed to find inactive users");
}
}--- a/backend/prisma/schema.prisma
+++ b/backend/prisma/schema.prisma
@@ model User {
status String
isEmailVerified Boolean
lastActivityAt DateTime?
lastReminderSent DateTime?
createdAt DateTime @default(now())
// existing indices...
+ @@index([status, isEmailVerified, lastReminderSent])
}📝 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.
| public static async findInactiveUsers(days: number) { | |
| const cutoffDate = new Date(); | |
| cutoffDate.setDate(cutoffDate.getDate() - days); | |
| try { | |
| return await prisma.user.findMany({ | |
| where: { | |
| status: Status.ACTIVE, | |
| isEmailVerified: true, | |
| OR: [ | |
| { | |
| lastActivityAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| { | |
| lastActivityAt: null, | |
| createdAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| firstName: true, | |
| lastName: true, | |
| lastActivityAt: true, | |
| lastReminderSent: true, | |
| createdAt: true, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to find inactive users:", error); | |
| throw new InternalError("Failed to find inactive users"); | |
| } | |
| } | |
| // File: backend/src/services/user.service.ts | |
| public static async findInactiveUsers( | |
| days: number, | |
| minHoursBetweenReminders = 24, | |
| ) { | |
| const cutoffDate = new Date(); | |
| cutoffDate.setDate(cutoffDate.getDate() - days); | |
| // Only re-remind users after at least minHoursBetweenReminders have passed | |
| const remindCutoff = new Date( | |
| Date.now() - minHoursBetweenReminders * 60 * 60 * 1000, | |
| ); | |
| try { | |
| return await prisma.user.findMany({ | |
| where: { | |
| status: Status.ACTIVE, | |
| isEmailVerified: true, | |
| AND: [ | |
| { | |
| OR: [ | |
| { lastActivityAt: { lte: cutoffDate } }, | |
| { lastActivityAt: null, createdAt: { lte: cutoffDate } }, | |
| ], | |
| }, | |
| { | |
| OR: [ | |
| { lastReminderSent: null }, | |
| { lastReminderSent: { lt: remindCutoff } }, | |
| ], | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| firstName: true, | |
| lastName: true, | |
| lastActivityAt: true, | |
| lastReminderSent: true, | |
| createdAt: true, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to find inactive users:", error); | |
| throw new InternalError("Failed to find inactive users"); | |
| } | |
| } |
| public static async findInactiveUsers(days: number) { | |
| const cutoffDate = new Date(); | |
| cutoffDate.setDate(cutoffDate.getDate() - days); | |
| try { | |
| return await prisma.user.findMany({ | |
| where: { | |
| status: Status.ACTIVE, | |
| isEmailVerified: true, | |
| OR: [ | |
| { | |
| lastActivityAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| { | |
| lastActivityAt: null, | |
| createdAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| ], | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| firstName: true, | |
| lastName: true, | |
| lastActivityAt: true, | |
| lastReminderSent: true, | |
| createdAt: true, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to find inactive users:", error); | |
| throw new InternalError("Failed to find inactive users"); | |
| } | |
| } | |
| // File: backend/prisma/schema.prisma | |
| model User { | |
| status String | |
| isEmailVerified Boolean | |
| lastActivityAt DateTime? | |
| lastReminderSent DateTime? | |
| createdAt DateTime @default(now()) | |
| // existing indices… | |
| @@index([status, isEmailVerified, lastReminderSent]) | |
| } |
| public static async countInactiveUsers(days: number): Promise<number> { | ||
| const cutoffDate = new Date(); | ||
| cutoffDate.setDate(cutoffDate.getDate() - days); | ||
|
|
||
| try { | ||
| return await prisma.user.count({ | ||
| where: { | ||
| status: Status.ACTIVE, | ||
| isEmailVerified: true, | ||
| OR: [ | ||
| { | ||
| lastActivityAt: { | ||
| lte: cutoffDate, | ||
| }, | ||
| }, | ||
| { | ||
| lastActivityAt: null, | ||
| createdAt: { | ||
| lte: cutoffDate, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to count inactive users:", error); | ||
| throw new InternalError("Failed to count inactive users"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep count in sync with candidate criteria (add cooldown predicate too)
countInactiveUsers should mirror findInactiveUsers to reflect the same cooldown and avoid misleading metrics.
- public static async countInactiveUsers(days: number): Promise<number> {
+ public static async countInactiveUsers(days: number, minHoursBetweenReminders = 24): Promise<number> {
const cutoffDate = new Date();
cutoffDate.setDate(cutoffDate.getDate() - days);
+ const remindCutoff = new Date(Date.now() - minHoursBetweenReminders * 60 * 60 * 1000);
try {
return await prisma.user.count({
where: {
status: Status.ACTIVE,
isEmailVerified: true,
- OR: [
- {
- lastActivityAt: {
- lte: cutoffDate,
- },
- },
- {
- lastActivityAt: null,
- createdAt: {
- lte: cutoffDate,
- },
- },
- ],
+ AND: [
+ {
+ OR: [
+ { lastActivityAt: { lte: cutoffDate } },
+ { lastActivityAt: null, createdAt: { lte: cutoffDate } },
+ ],
+ },
+ {
+ OR: [
+ { lastReminderSent: null },
+ { lastReminderSent: { lt: remindCutoff } },
+ ],
+ },
+ ],
},
});📝 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.
| public static async countInactiveUsers(days: number): Promise<number> { | |
| const cutoffDate = new Date(); | |
| cutoffDate.setDate(cutoffDate.getDate() - days); | |
| try { | |
| return await prisma.user.count({ | |
| where: { | |
| status: Status.ACTIVE, | |
| isEmailVerified: true, | |
| OR: [ | |
| { | |
| lastActivityAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| { | |
| lastActivityAt: null, | |
| createdAt: { | |
| lte: cutoffDate, | |
| }, | |
| }, | |
| ], | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to count inactive users:", error); | |
| throw new InternalError("Failed to count inactive users"); | |
| } | |
| } | |
| public static async countInactiveUsers( | |
| days: number, | |
| minHoursBetweenReminders = 24 | |
| ): Promise<number> { | |
| const cutoffDate = new Date(); | |
| cutoffDate.setDate(cutoffDate.getDate() - days); | |
| const remindCutoff = new Date(Date.now() - minHoursBetweenReminders * 60 * 60 * 1000); | |
| try { | |
| return await prisma.user.count({ | |
| where: { | |
| status: Status.ACTIVE, | |
| isEmailVerified: true, | |
| AND: [ | |
| { | |
| OR: [ | |
| { lastActivityAt: { lte: cutoffDate } }, | |
| { lastActivityAt: null, createdAt: { lte: cutoffDate } }, | |
| ], | |
| }, | |
| { | |
| OR: [ | |
| { lastReminderSent: null }, | |
| { lastReminderSent: { lt: remindCutoff } }, | |
| ], | |
| }, | |
| ], | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to count inactive users:", error); | |
| throw new InternalError("Failed to count inactive users"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/src/services/user.service.ts around lines 131 to 159, the where
clause used for counting inactive users does not include the cooldown predicate
and therefore diverges from findInactiveUsers; update the prisma.user.count
where filter to exactly mirror the predicates used in findInactiveUsers (status,
isEmailVerified, the same OR on lastActivityAt/createdAt AND include the
cooldown check used there) so metrics match the selection logic — copy the same
cooldown condition (same field name and comparison) and combine it with the
existing predicates so countInactiveUsers returns the same set as
findInactiveUsers.
|
Hello @laryhills. Please address every coderabbit suggestion. |
|
Hello @laryhills please pull the changes and rebase and fix conflicts sir. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/utils/service/nodeMailer.ts (1)
45-50: Fix logs that never print messageId/error.Template strings contain literal “info.messageId”/“error” instead of variables.
Apply:
- console.log(`📨 Text email sent to ${email}:, info.messageId`); + console.log(`📨 Text email sent to ${email}: ${info.messageId}`); ... - console.error(`❌ Failed to send text email to ${email}:, error`); + console.error(`❌ Failed to send text email to ${email}:`, error); ... - console.log(`📧 HTML email sent to ${email}:, info.messageId`); + console.log(`📧 HTML email sent to ${email}: ${info.messageId}`); ... - console.error(`❌ Failed to send HTML email to ${email}:, error`); + console.error(`❌ Failed to send HTML email to ${email}:`, error);Consider masking PII in logs (e.g., j***@d***.com).
Also applies to: 63-69
🧹 Nitpick comments (6)
backend/src/utils/service/nodeMailer.ts (1)
11-16: Avoid hardcoding Gmail; drive SMTP from config (and class name mismatch).Reading host/port/secure from settings keeps environments flexible and prevents accidental prod breakage. Also, the class is named ZohoMailer but now points to Gmail; consider renaming to SmtpMailer for clarity.
Apply:
- host: "smtp.gmail.com", - port: 587, - secure: false, + host: serverSettings.email.host, + port: serverSettings.email.port ?? 587, + secure: serverSettings.email.secure ?? false, + requireTLS: serverSettings.email.requireTLS ?? true, connectionTimeout: 10000, // 10 seconds greetingTimeout: 10000, // 10 seconds socketTimeout: 10000, // 10 secondsOptional (helps throughput, fewer handshakes):
- this.transporter = nodemailer.createTransport({ + this.transporter = nodemailer.createTransport({ + pool: serverSettings.email.pool ?? true, + maxConnections: serverSettings.email.maxConnections ?? 3, + maxMessages: serverSettings.email.maxMessages ?? 100,backend/src/utils/service/emailNotifier.ts (5)
11-23: LGTM: validated app URL.Good defensive check; matches prior feedback. Consider sourcing from centralized settings to avoid direct env reads in services.
99-105: Reuse a shared mailer; avoid per-email SMTP handshakes.Instantiating ZohoMailer for every email is costly and risks throttling. Keep a static/shared instance.
Apply:
- const mailer = new ZohoMailer(); - - await mailer.sendTextEmail(data.email, subject, message); + await EmailNotifier.mailer.sendTextEmail(data.email, subject, message);Add once in this class (outside the shown range):
// inside class EmailNotifier private static mailer = new ZohoMailer();Optional: prefer HTML for better UX (clickable link) via sendHtmlEmail.
33-97: Reduce template duplication; optional HTML templates.The switch repeats structure. A map improves maintainability; HTML templates would also allow richer content and tracking links.
Example refactor:
- switch (reminderType) { - case '7-day': - return { subject: "...", message: `... ${this.getValidatedAppUrl()}` }; - ... - default: - throw new Error(`Unknown reminder type: ${reminderType}`); - } + const name = firstName || "there"; + const appUrl = this.getValidatedAppUrl(); + const templates: Record<ReminderEmailData['reminderType'], {subject:string; message:string}> = { + '7-day': { subject: "We miss you! Come back to AURORA", message: `Hi ${name},\n\nWe noticed ... ${appUrl}\n\nKeep learning!\nThe AURORA Team` }, + '14-day': { subject: "Don't lose your progress - Continue your journey", message: `Hi ${name},\n\nIt's been ${daysSinceActivity} days ... ${appUrl}\n\nWe believe ...` }, + '30-day': { subject: "Special learning resources waiting for you", message: `Hi ${name},\n\nWe haven't seen you ... ${appUrl}\n\nLet's restart ...` }, + '60-day': { subject: "Your journey awaits - Final reminder", message: `Hi ${name},\n\nIt's been ${daysSinceActivity} days ... ${appUrl}\n\nWishing you success,\nThe AURORA Team` } + }; + return templates[reminderType];
1-1: Naming mismatch: ZohoMailer now uses Gmail.Rename to
SmtpMailer(and update imports) to avoid future confusion.
112-152: Clarify retry semantics and consider bounded concurrency
The current loop (
while (attempts <= maxRetries)) treatsmaxRetriesas number of retries (so with the default of 2 it actually attempts up to 3 sends: 1 initial + 2 retries). If that’s intended, leave it asmaxRetriesbut add a comment explaining this. Otherwise, you can:
- Rename the parameter to
maxAttempts(and update all call‐sites inuserReminder.job.tsandsettings.ts), then change towhile (attempts < maxAttempts).- Or keep the name
maxRetriesand change the loop towhile (attempts < maxRetries)if you truly want “total attempts = maxRetries.”For large batches, sequential awaits can be slow. Consider a bounded‐concurrency approach (e.g., use a promise‐pool or libraries like
p-mapwith a concurrency of 5–10) so you parallelizesendReminderEmailcalls without overwhelming the SMTP server.Example optional diff if you opt for
maxAttemptssemantics:- public static async sendReminderEmails(reminders: ReminderEmailData[], maxRetries: number = 2, retryDelayMs: number = 1000) { + public static async sendReminderEmails( + reminders: ReminderEmailData[], + maxAttempts: number = 3, // total send attempts + retryDelayMs: number = 1000 + ) { … - while (attempts <= maxRetries && !success) { + while (attempts < maxAttempts && !success) { … - if (attempts > maxRetries) { + if (attempts >= maxAttempts) { results.failed++; … }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/src/utils/service/emailNotifier.ts(2 hunks)backend/src/utils/service/nodeMailer.ts(1 hunks)
|
@Gerson2102 Will do so as soon as possible |
|
@Gerson2102 |
3 participants
Pull Request for STARKLA - Close Issue
❗ Pull Request Information
This PR Implements email notification reminders for inactive users to try to re-engage them.
Add here some information
🗒️ Summary of Changes
lastLoginAtandlastActivityAtfields.env.exmaple⚒️ Testing
Evidence After Solution
📂 Related Issue
This pull request will **close #49 ** upon merging.
🎉 Thank you for reviewing this PR! 🎉
Summary by CodeRabbit