feat: implement automated email and push notification service#162
Open
Ndifreke000 wants to merge 2 commits intoanonfedora:masterfrom
Open
feat: implement automated email and push notification service#162Ndifreke000 wants to merge 2 commits intoanonfedora:masterfrom
Ndifreke000 wants to merge 2 commits intoanonfedora:masterfrom
Conversation
Closes anonfedora#142 - Integrated SendGrid for email delivery with HTML templates - Created NotificationService handling both WebSocket and Email channels - Implemented 8 notification types: Loan Approved, Repayment Due, Escrow Expiring, Escrow Released, Security Alert, Collateral Locked, Oracle Confirmation, Governance Proposal - Added HTML email templates for all notification types with responsive design - Implemented 60-second SLA for notification delivery with monitoring - Added user notification preferences model to database schema - Users can opt-in/out of notification categories (except security alerts) - Integrated with existing WebSocket service for real-time notifications - Added notification queue with async processing to prevent blocking - Integrated with loan service to send approval notifications - Email templates include actionable links and transaction details - Added event emitter for notification monitoring and analytics - Configured SendGrid API key and sender email in environment variables
|
@Ndifreke000 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdded a queued NotificationService with SendGrid email and WebSocket delivery, a NotificationPreference model linked to User, SendGrid configuration and dependency, and notification dispatch integrated into loan status transitions. Changes
Sequence DiagramsequenceDiagram
participant Loan as "Loan Service"
participant Notif as "Notification Service"
participant DB as "Database (Prisma)"
participant Queue as "Queue"
participant SG as "SendGrid"
participant WS as "WebSocket"
Loan->>Notif: sendNotification(payload)
activate Notif
Notif->>DB: getUserPreferences(userId)
DB-->>Notif: preferences
alt user allowed
Notif->>Queue: enqueue(payload)
Note right of Queue: background processing
Queue->>Notif: processQueue(item)
activate Notif
par deliver
Notif->>DB: findUser(userId)
DB-->>Notif: user data
Notif->>SG: sgMail.send(emailPayload)
SG-->>Notif: send result
and
Notif->>WS: emit(notification)
WS-->>Notif: ack
end
Notif->>Notif: emit notification:delivered
deactivate Notif
else opted out
Notif-->>Loan: skipped (preference)
end
deactivate Notif
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/services/loan.service.ts`:
- Line 251: Replace the direct process.env access in the dashboardUrl
construction with the centralized config object imported as env (i.e., use
env.FRONTEND_URL instead of process.env.FRONTEND_URL) in the code that
constructs dashboardUrl for the loan (the object building dashboardUrl:
`${...}/loans/${loanId}` inside loan.service.ts); ensure you keep the same
fallback/default behavior currently provided and reference the env property used
elsewhere in this file for consistency.
- Around line 238-254: The sendNotification call inside db.$transaction is
blocking and can cause transaction timeouts or rollbacks; capture the
notification payload and a flag (e.g., shouldNotifyApproval and
notificationData) inside the transaction (using existing symbols like loan,
loanId, NotificationType.LOAN_APPROVED, NotificationChannel.BOTH) but do NOT
call notificationService.sendNotification there; after the db.$transaction
completes, call notificationService.sendNotification with the captured payload
(and wrap it in a .catch to log failures) so the DB commit is not tied to
external delivery.
In `@server/src/services/notification.service.ts`:
- Around line 333-334: User-provided fields like data.description and data.title
are injected directly into the email HTML; add an escapeHtml(unsafe: string):
string utility that HTML-escapes & < > " ' and apply it to all user-controlled
text interpolations in the notification template (e.g., replace
${data.description} and ${data.title} with ${escapeHtml(data.description)} /
${escapeHtml(data.title)} in the code that builds the HTML in
notification.service.ts), and ensure any other user-provided fields used in text
nodes or attributes are either escaped with escapeHtml or properly
validated/URL-encoded (e.g., voteUrl) before interpolation.
- Around line 385-392: The updatePreferences method currently only logs and
emits an event; modify NotificationService.updatePreferences to persist the
provided Partial<Record<NotificationType, boolean>> into the
NotificationPreference table (upserting or creating one row per NotificationType
for the given userId) before emitting the "preferences:updated" event; ensure
you map missing keys to their current values or defaults, perform the DB writes
via the existing NotificationPreference repository/ORM within a transaction,
handle and surface DB errors (rejecting the promise) and only emit the event
after successful persistence.
- Around line 115-125: The code uses Promise.allSettled(promises) but never
inspects the results, so even if all delivery attempts (the promises array used
for WebSocket/email sends) fail it still logs success and emits
this.emit("notification:delivered", { userId, type, duration });; update the
block that awaits Promise.allSettled to capture its result, check each
settlement for status === "fulfilled" (or count/reduce successes), and only
log/emit delivery when at least one channel succeeded—otherwise log/emit a
failure event (e.g., "notification:failed") or include error details; keep
NOTIFICATION_TIMEOUT_MS duration handling as is and use the same userId, type,
startTime symbols to compute and report duration.
- Around line 348-361: getUserPreferences currently returns hardcoded defaults
instead of reading the NotificationPreference table; update the method
(getUserPreferences) to query the NotificationPreference model for the given
userId (e.g., NotificationPreference.findAll / repository query), build and
return a Record<string, boolean> mapping of NotificationType keys to the user's
stored preference values, and fall back to true for any notification types not
present in the DB; ensure you import/use the correct DB client or model where
NotificationPreference is defined and keep the method async/Promise-based as it
is now.
- Around line 375-380: The startProcessor method creates an uncancelled
setInterval causing leaked timers; modify NotificationService to save the
returned interval handle (from startProcessor) to a private field (e.g.,
this.processorInterval), add a stopProcessor or dispose method that calls
clearInterval(this.processorInterval) and nulls the field, and ensure any
re-initialization path (or service shutdown) calls that stop method before
creating a new interval; update references to startProcessor/processQueue
accordingly so the interval can be cleaned up.
- Around line 163-165: The service currently fabricates emails from
user.stellarAddress which will never deliver; update the codepath so email
notifications only use a real User.email or are skipped: add a required email
field to the User model (User interface/class) and ensure user
creation/validation populates it, or modify sendEmailNotification (and any
callers) to check for a valid email (e.g., user.email present and matches a
simple email regex) and return/skip if missing; update any tests and the
notification sending call sites to rely on user.email instead of the fabricated
user.stellarAddress.substring(...) value.
- Around line 128-141: sendWebSocketNotification currently calls a non-existent
websocketService.broadcast() and would leak notifications to all clients because
WsState.broadcastEvent() broadcasts globally; change sendWebSocketNotification
to call a user-targeted send (e.g., use websocketService.getWsState() to look up
the connection(s) for the given userId and invoke a sendToConnection/sendToUser
helper or an existing specific method like
broadcastEscrowCreated/broadcastEscrowUpdated after adapting them to accept a
target userId), and update WsState by adding a user-to-connection mapping and a
sendToUser/sendToConnections(userId, message) method (or change broadcastEvent
to accept an optional userId filter) so messages are delivered only to the
intended recipient(s) rather than all connected clients.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ae5464d-717d-4cf3-860a-ccb7d13d390f
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
server/.env.exampleserver/package.jsonserver/prisma/schema.prismaserver/src/services/loan.service.tsserver/src/services/notification.service.ts
Comment on lines
+238
to
+254
|
|
||
| // Send notification if loan is approved (moved to ACTIVE) | ||
| if (nextStatus === "ACTIVE") { | ||
| await notificationService.sendNotification({ | ||
| userId: loan.borrowerId, | ||
| type: NotificationType.LOAN_APPROVED, | ||
| channel: NotificationChannel.BOTH, | ||
| data: { | ||
| loanId, | ||
| amount: loan.amount.toString(), | ||
| assetCode: loan.assetCode, | ||
| interestRate: loan.interestRate.toString(), | ||
| dueDate: loan.dueDate?.toISOString() || "N/A", | ||
| dashboardUrl: `${process.env.FRONTEND_URL || "https://stellovault.com"}/loans/${loanId}`, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Move notification dispatch outside the database transaction to avoid blocking and potential rollback.
The await notificationService.sendNotification() call is inside the db.$transaction() block (lines 187-272). If SendGrid is slow or fails, this will:
- Hold the serializable transaction open unnecessarily
- Risk transaction timeout under load
- Potentially roll back a successful database update due to email delivery failure
The notification should be queued after the transaction commits successfully.
🛠️ Proposed fix: dispatch notification after transaction
- if (nextStatus !== loan.status) {
- await tx.loan.update({
- where: { id: loanId },
- data: { status: nextStatus },
- });
-
- websocketService.broadcastLoanUpdated(loanId, nextStatus);
-
- // Send notification if loan is approved (moved to ACTIVE)
- if (nextStatus === "ACTIVE") {
- await notificationService.sendNotification({
- userId: loan.borrowerId,
- type: NotificationType.LOAN_APPROVED,
- channel: NotificationChannel.BOTH,
- data: {
- loanId,
- amount: loan.amount.toString(),
- assetCode: loan.assetCode,
- interestRate: loan.interestRate.toString(),
- dueDate: loan.dueDate?.toISOString() || "N/A",
- dashboardUrl: `${process.env.FRONTEND_URL || "https://stellovault.com"}/loans/${loanId}`,
- },
- });
- }
- }
+ let shouldNotifyApproval = false;
+ let notificationData: { borrowerId: string; loanId: string; amount: string; assetCode: string; interestRate: string; dueDate: string } | null = null;
+
+ if (nextStatus !== loan.status) {
+ await tx.loan.update({
+ where: { id: loanId },
+ data: { status: nextStatus },
+ });
+
+ websocketService.broadcastLoanUpdated(loanId, nextStatus);
+
+ if (nextStatus === "ACTIVE") {
+ shouldNotifyApproval = true;
+ notificationData = {
+ borrowerId: loan.borrowerId,
+ loanId,
+ amount: loan.amount.toString(),
+ assetCode: loan.assetCode,
+ interestRate: loan.interestRate.toString(),
+ dueDate: loan.dueDate?.toISOString() || "N/A",
+ };
+ }
+ }Then after the transaction block (after line 272), add:
// Send notification after transaction commits
if (shouldNotifyApproval && notificationData) {
notificationService.sendNotification({
userId: notificationData.borrowerId,
type: NotificationType.LOAN_APPROVED,
channel: NotificationChannel.BOTH,
data: {
...notificationData,
dashboardUrl: `${env.frontendUrl || "https://stellovault.com"}/loans/${notificationData.loanId}`,
},
}).catch(err => console.error("Failed to send loan approval notification:", err));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/loan.service.ts` around lines 238 - 254, The
sendNotification call inside db.$transaction is blocking and can cause
transaction timeouts or rollbacks; capture the notification payload and a flag
(e.g., shouldNotifyApproval and notificationData) inside the transaction (using
existing symbols like loan, loanId, NotificationType.LOAN_APPROVED,
NotificationChannel.BOTH) but do NOT call notificationService.sendNotification
there; after the db.$transaction completes, call
notificationService.sendNotification with the captured payload (and wrap it in a
.catch to log failures) so the DB commit is not tied to external delivery.
| assetCode: loan.assetCode, | ||
| interestRate: loan.interestRate.toString(), | ||
| dueDate: loan.dueDate?.toISOString() || "N/A", | ||
| dashboardUrl: `${process.env.FRONTEND_URL || "https://stellovault.com"}/loans/${loanId}`, |
There was a problem hiding this comment.
Use the env config object for consistency.
The file imports env from "../config/env" (line 8) and uses it elsewhere (e.g., line 102), but this line accesses process.env.FRONTEND_URL directly. Using the centralized config ensures consistent validation and defaults.
- dashboardUrl: `${process.env.FRONTEND_URL || "https://stellovault.com"}/loans/${loanId}`,
+ dashboardUrl: `${env.frontendUrl || "https://stellovault.com"}/loans/${loanId}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/loan.service.ts` at line 251, Replace the direct
process.env access in the dashboardUrl construction with the centralized config
object imported as env (i.e., use env.FRONTEND_URL instead of
process.env.FRONTEND_URL) in the code that constructs dashboardUrl for the loan
(the object building dashboardUrl: `${...}/loans/${loanId}` inside
loan.service.ts); ensure you keep the same fallback/default behavior currently
provided and reference the env property used elsewhere in this file for
consistency.
Comment on lines
+115
to
+125
| await Promise.allSettled(promises); | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| console.log(`Notification ${type} delivered to user ${userId} in ${duration}ms`); | ||
|
|
||
| // Check if within 60s SLA | ||
| if (duration > NOTIFICATION_TIMEOUT_MS) { | ||
| console.warn(`⚠️ Notification ${type} exceeded 60s SLA: ${duration}ms`); | ||
| } | ||
|
|
||
| this.emit("notification:delivered", { userId, type, duration }); |
There was a problem hiding this comment.
notification:delivered event emitted even when both delivery channels fail.
Promise.allSettled always resolves, but the results aren't inspected. If both WebSocket and email delivery fail, the code still emits notification:delivered and logs success, masking failures from monitoring.
♻️ Proposed fix: Check settlement results
- await Promise.allSettled(promises);
+ const results = await Promise.allSettled(promises);
+ const failures = results.filter(r => r.status === "rejected");
+ const successes = results.filter(r => r.status === "fulfilled");
const duration = Date.now() - startTime;
- console.log(`Notification ${type} delivered to user ${userId} in ${duration}ms`);
// Check if within 60s SLA
if (duration > NOTIFICATION_TIMEOUT_MS) {
console.warn(`⚠️ Notification ${type} exceeded 60s SLA: ${duration}ms`);
}
- this.emit("notification:delivered", { userId, type, duration });
+ if (failures.length === results.length) {
+ console.error(`✗ All delivery channels failed for ${type} to user ${userId}`);
+ this.emit("notification:failed", {
+ payload: { userId, type, channel, data },
+ error: new Error("All delivery channels failed")
+ });
+ } else {
+ console.log(`Notification ${type} delivered to user ${userId} in ${duration}ms (${successes.length}/${results.length} channels)`);
+ this.emit("notification:delivered", { userId, type, duration, partialFailure: failures.length > 0 });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 115 - 125, The code
uses Promise.allSettled(promises) but never inspects the results, so even if all
delivery attempts (the promises array used for WebSocket/email sends) fail it
still logs success and emits this.emit("notification:delivered", { userId, type,
duration });; update the block that awaits Promise.allSettled to capture its
result, check each settlement for status === "fulfilled" (or count/reduce
successes), and only log/emit delivery when at least one channel
succeeded—otherwise log/emit a failure event (e.g., "notification:failed") or
include error details; keep NOTIFICATION_TIMEOUT_MS duration handling as is and
use the same userId, type, startTime symbols to compute and report duration.
Comment on lines
+128
to
+141
| private async sendWebSocketNotification( | ||
| userId: string, | ||
| type: NotificationType, | ||
| data: Record<string, any> | ||
| ): Promise<void> { | ||
| // Send real-time notification via WebSocket | ||
| websocketService.broadcast({ | ||
| type: "NOTIFICATION", | ||
| notificationType: type, | ||
| userId, | ||
| data, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify websocketService.broadcast method and WsState implementation
echo "=== Checking websocket service broadcast implementation ==="
rg -n "broadcast" server/src/services/websocket.service.ts -A 10
echo ""
echo "=== Checking if there's any user-to-connection mapping ==="
rg -n "userId|user.*connection|connection.*user" server/src/services/websocket.service.tsRepository: anonfedora/stellovault
Length of output: 1842
🏁 Script executed:
# Check the notification service file to see the actual sendWebSocketNotification method
head -150 server/src/services/notification.service.ts | tail -50Repository: anonfedora/stellovault
Length of output: 1753
🏁 Script executed:
# Get the complete websocket.service.ts to understand the full implementation
wc -l server/src/services/websocket.service.tsRepository: anonfedora/stellovault
Length of output: 110
🏁 Script executed:
# Check if there's a broadcast method in websocket.service.ts (not broadcastEvent)
rg -n "^\s*(public|private).*broadcast\(" server/src/services/websocket.service.tsRepository: anonfedora/stellovault
Length of output: 48
🏁 Script executed:
# Read the complete websocket.service.ts to understand all methods
cat -n server/src/services/websocket.service.tsRepository: anonfedora/stellovault
Length of output: 4887
🏁 Script executed:
# Check if broadcast() method is defined anywhere else in the codebase
rg -n "broadcast\(" server/src/ --type tsRepository: anonfedora/stellovault
Length of output: 150
🏁 Script executed:
# Check notification.service.ts imports to see if broadcast comes from elsewhere
head -30 server/src/services/notification.service.tsRepository: anonfedora/stellovault
Length of output: 991
🏁 Script executed:
# Verify no dynamic method assignment or similar patterns
rg -n "broadcast\s*=" server/src/services/websocket.service.tsRepository: anonfedora/stellovault
Length of output: 48
websocketService.broadcast() does not exist—this code will fail at runtime.
The WebSocketService class has no broadcast() method. It only exposes getWsState() and specific broadcast methods like broadcastEscrowCreated(), broadcastEscrowUpdated(), etc. Calling websocketService.broadcast() at line 134 will throw a TypeError.
Beyond the broken method call, the underlying architecture has a critical privacy issue: WsState.broadcastEvent() (lines 81–88 in websocket.service.ts) sends messages to all connected clients without user filtering. Even if the method call were fixed, notifications would leak to all users rather than only the intended recipient. User-specific data (loan details, security alerts) would be visible to every connected client.
This requires both immediate fixes: correct the method call and implement user-targeted delivery using either user-to-connection mapping in WsState or a pub/sub pattern with user-specific channels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 128 - 141,
sendWebSocketNotification currently calls a non-existent
websocketService.broadcast() and would leak notifications to all clients because
WsState.broadcastEvent() broadcasts globally; change sendWebSocketNotification
to call a user-targeted send (e.g., use websocketService.getWsState() to look up
the connection(s) for the given userId and invoke a sendToConnection/sendToUser
helper or an existing specific method like
broadcastEscrowCreated/broadcastEscrowUpdated after adapting them to accept a
target userId), and update WsState by adding a user-to-connection mapping and a
sendToUser/sendToConnections(userId, message) method (or change broadcastEvent
to accept an optional userId filter) so messages are delivered only to the
intended recipient(s) rather than all connected clients.
Comment on lines
+163
to
+165
| // For demo purposes, use stellar address as email | ||
| // In production, you'd have a separate email field | ||
| const userEmail = `${user.stellarAddress.substring(0, 8)}@example.com`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if User model has an email field
echo "=== Checking User model fields in schema ==="
rg -n "model User" server/prisma/schema.prisma -A 20Repository: anonfedora/stellovault
Length of output: 1066
🏁 Script executed:
#!/bin/bash
# Read the notification service around lines 163-165
echo "=== Notification service at lines 155-175 ==="
sed -n '155,175p' server/src/services/notification.service.tsRepository: anonfedora/stellovault
Length of output: 783
🏁 Script executed:
#!/bin/bash
# Check the NotificationPreference model for email field
echo "=== Checking NotificationPreference model ==="
rg -n "model NotificationPreference" server/prisma/schema.prisma -A 15Repository: anonfedora/stellovault
Length of output: 882
🏁 Script executed:
#!/bin/bash
# Search for actual email sending implementation
echo "=== Looking for email sending logic ==="
rg -n "sendEmail|transporter|nodemailer|mail" server/src/services/notification.service.ts -B 2 -A 5Repository: anonfedora/stellovault
Length of output: 2647
Fake email addresses will fail to deliver notifications as required.
The notification service constructs fabricated emails (${stellarAddress.substring(0, 8)}@example.com) and sends them via SendGrid (line 178), but these addresses are invalid and will never reach users. This conflicts with the PR requirement for email delivery within 60 seconds of state changes.
Add an email field to the User model as a required field for email notifications, or gate the sendEmailNotification method to skip sending when no valid user email exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 163 - 165, The
service currently fabricates emails from user.stellarAddress which will never
deliver; update the codepath so email notifications only use a real User.email
or are skipped: add a required email field to the User model (User
interface/class) and ensure user creation/validation populates it, or modify
sendEmailNotification (and any callers) to check for a valid email (e.g.,
user.email present and matches a simple email regex) and return/skip if missing;
update any tests and the notification sending call sites to rely on user.email
instead of the fabricated user.stellarAddress.substring(...) value.
Comment on lines
+333
to
+334
| <p>${data.description}</p> | ||
| <p><a href="${data.voteUrl}">Cast Your Vote</a></p> |
There was a problem hiding this comment.
User-controlled content is interpolated directly into HTML without escaping.
${data.description} (governance proposal description) and similar fields like ${data.title} are user-provided and inserted directly into HTML. A malicious user could inject HTML/JavaScript that some email clients might render, potentially leading to phishing or tracking attacks.
Consider escaping HTML entities in user-controlled fields:
🛡️ Proposed fix: Add HTML escaping utility
function escapeHtml(unsafe: string): string {
return unsafe
.replace(/&/g, "&")
.replace(/</g, "<")
.replace(/>/g, ">")
.replace(/"/g, """)
.replace(/'/g, "'");
}
// Then use it for user-controlled fields:
<p>${escapeHtml(data.description)}</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 333 - 334,
User-provided fields like data.description and data.title are injected directly
into the email HTML; add an escapeHtml(unsafe: string): string utility that
HTML-escapes & < > " ' and apply it to all user-controlled text interpolations
in the notification template (e.g., replace ${data.description} and
${data.title} with ${escapeHtml(data.description)} / ${escapeHtml(data.title)}
in the code that builds the HTML in notification.service.ts), and ensure any
other user-provided fields used in text nodes or attributes are either escaped
with escapeHtml or properly validated/URL-encoded (e.g., voteUrl) before
interpolation.
Comment on lines
+348
to
+361
| private async getUserPreferences(userId: string): Promise<Record<string, boolean>> { | ||
| // In production, fetch from database | ||
| // For now, return default preferences (all enabled) | ||
| return { | ||
| [NotificationType.LOAN_APPROVED]: true, | ||
| [NotificationType.LOAN_REPAYMENT_DUE]: true, | ||
| [NotificationType.ESCROW_EXPIRING_SOON]: true, | ||
| [NotificationType.ESCROW_RELEASED]: true, | ||
| [NotificationType.SECURITY_ALERT]: true, | ||
| [NotificationType.COLLATERAL_LOCKED]: true, | ||
| [NotificationType.ORACLE_CONFIRMATION]: true, | ||
| [NotificationType.GOVERNANCE_PROPOSAL]: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
User preferences are never fetched from the database.
getUserPreferences returns hardcoded defaults instead of querying the NotificationPreference table. Users who update their preferences will still receive all notifications, violating the PR objective for user opt-in/out controls.
🛠️ Proposed fix: Query the database for user preferences
private async getUserPreferences(userId: string): Promise<Record<string, boolean>> {
- // In production, fetch from database
- // For now, return default preferences (all enabled)
- return {
- [NotificationType.LOAN_APPROVED]: true,
- [NotificationType.LOAN_REPAYMENT_DUE]: true,
- [NotificationType.ESCROW_EXPIRING_SOON]: true,
- [NotificationType.ESCROW_RELEASED]: true,
- [NotificationType.SECURITY_ALERT]: true,
- [NotificationType.COLLATERAL_LOCKED]: true,
- [NotificationType.ORACLE_CONFIRMATION]: true,
- [NotificationType.GOVERNANCE_PROPOSAL]: true,
- };
+ const prefs = await prisma.notificationPreference.findUnique({
+ where: { userId },
+ });
+
+ // Map database fields to NotificationType enum values
+ if (prefs) {
+ return {
+ [NotificationType.LOAN_APPROVED]: prefs.loanApproved,
+ [NotificationType.LOAN_REPAYMENT_DUE]: prefs.loanRepaymentDue,
+ [NotificationType.ESCROW_EXPIRING_SOON]: prefs.escrowExpiringSoon,
+ [NotificationType.ESCROW_RELEASED]: prefs.escrowReleased,
+ [NotificationType.SECURITY_ALERT]: prefs.securityAlert,
+ [NotificationType.COLLATERAL_LOCKED]: prefs.collateralLocked,
+ [NotificationType.ORACLE_CONFIRMATION]: prefs.oracleConfirmation,
+ [NotificationType.GOVERNANCE_PROPOSAL]: prefs.governanceProposal,
+ };
+ }
+
+ // Return defaults if no preferences record exists
+ return {
+ [NotificationType.LOAN_APPROVED]: true,
+ [NotificationType.LOAN_REPAYMENT_DUE]: true,
+ [NotificationType.ESCROW_EXPIRING_SOON]: true,
+ [NotificationType.ESCROW_RELEASED]: true,
+ [NotificationType.SECURITY_ALERT]: true,
+ [NotificationType.COLLATERAL_LOCKED]: true,
+ [NotificationType.ORACLE_CONFIRMATION]: true,
+ [NotificationType.GOVERNANCE_PROPOSAL]: true,
+ };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 348 - 361,
getUserPreferences currently returns hardcoded defaults instead of reading the
NotificationPreference table; update the method (getUserPreferences) to query
the NotificationPreference model for the given userId (e.g.,
NotificationPreference.findAll / repository query), build and return a
Record<string, boolean> mapping of NotificationType keys to the user's stored
preference values, and fall back to true for any notification types not present
in the DB; ensure you import/use the correct DB client or model where
NotificationPreference is defined and keep the method async/Promise-based as it
is now.
Comment on lines
+375
to
+380
| private startProcessor(): void { | ||
| // Process queue every 5 seconds | ||
| setInterval(() => { | ||
| this.processQueue(); | ||
| }, 5000); | ||
| } |
There was a problem hiding this comment.
Interval is never cleared, causing potential memory leak.
The setInterval in startProcessor runs indefinitely. If the NotificationService instance is recreated (e.g., during hot-reload in development or if the module is re-imported), multiple intervals will accumulate. Store the interval handle and provide a cleanup method.
♻️ Proposed fix
export class NotificationService extends EventEmitter {
private notificationQueue: NotificationPayload[] = [];
private processing = false;
+ private processorInterval: NodeJS.Timeout | null = null;
constructor() {
super();
this.startProcessor();
}
private startProcessor(): void {
// Process queue every 5 seconds
- setInterval(() => {
+ this.processorInterval = setInterval(() => {
this.processQueue();
}, 5000);
}
+ cleanup(): void {
+ if (this.processorInterval) {
+ clearInterval(this.processorInterval);
+ this.processorInterval = null;
+ }
+ }
}📝 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.
Suggested change
| private startProcessor(): void { | |
| // Process queue every 5 seconds | |
| setInterval(() => { | |
| this.processQueue(); | |
| }, 5000); | |
| } | |
| export class NotificationService extends EventEmitter { | |
| private notificationQueue: NotificationPayload[] = []; | |
| private processing = false; | |
| private processorInterval: NodeJS.Timeout | null = null; | |
| constructor() { | |
| super(); | |
| this.startProcessor(); | |
| } | |
| private startProcessor(): void { | |
| // Process queue every 5 seconds | |
| this.processorInterval = setInterval(() => { | |
| this.processQueue(); | |
| }, 5000); | |
| } | |
| cleanup(): void { | |
| if (this.processorInterval) { | |
| clearInterval(this.processorInterval); | |
| this.processorInterval = null; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 375 - 380, The
startProcessor method creates an uncancelled setInterval causing leaked timers;
modify NotificationService to save the returned interval handle (from
startProcessor) to a private field (e.g., this.processorInterval), add a
stopProcessor or dispose method that calls clearInterval(this.processorInterval)
and nulls the field, and ensure any re-initialization path (or service shutdown)
calls that stop method before creating a new interval; update references to
startProcessor/processQueue accordingly so the interval can be cleaned up.
Comment on lines
+385
to
+392
| async updatePreferences( | ||
| userId: string, | ||
| preferences: Partial<Record<NotificationType, boolean>> | ||
| ): Promise<void> { | ||
| // In production, save to database | ||
| console.log(`Updated preferences for user ${userId}:`, preferences); | ||
| this.emit("preferences:updated", { userId, preferences }); | ||
| } |
There was a problem hiding this comment.
Preferences are not persisted to the database.
updatePreferences only logs and emits an event without saving to the NotificationPreference table. User preference changes will be lost on restart, and the acceptance criteria "Users must be able to successfully update their communication preferences" is not met.
🛠️ Proposed fix: Persist preferences to database
async updatePreferences(
userId: string,
preferences: Partial<Record<NotificationType, boolean>>
): Promise<void> {
- // In production, save to database
- console.log(`Updated preferences for user ${userId}:`, preferences);
+ // Map NotificationType enum to database field names
+ const dbPrefs: Record<string, boolean> = {};
+ if (preferences[NotificationType.LOAN_APPROVED] !== undefined) {
+ dbPrefs.loanApproved = preferences[NotificationType.LOAN_APPROVED];
+ }
+ if (preferences[NotificationType.LOAN_REPAYMENT_DUE] !== undefined) {
+ dbPrefs.loanRepaymentDue = preferences[NotificationType.LOAN_REPAYMENT_DUE];
+ }
+ if (preferences[NotificationType.ESCROW_EXPIRING_SOON] !== undefined) {
+ dbPrefs.escrowExpiringSoon = preferences[NotificationType.ESCROW_EXPIRING_SOON];
+ }
+ if (preferences[NotificationType.ESCROW_RELEASED] !== undefined) {
+ dbPrefs.escrowReleased = preferences[NotificationType.ESCROW_RELEASED];
+ }
+ if (preferences[NotificationType.SECURITY_ALERT] !== undefined) {
+ dbPrefs.securityAlert = preferences[NotificationType.SECURITY_ALERT];
+ }
+ if (preferences[NotificationType.COLLATERAL_LOCKED] !== undefined) {
+ dbPrefs.collateralLocked = preferences[NotificationType.COLLATERAL_LOCKED];
+ }
+ if (preferences[NotificationType.ORACLE_CONFIRMATION] !== undefined) {
+ dbPrefs.oracleConfirmation = preferences[NotificationType.ORACLE_CONFIRMATION];
+ }
+ if (preferences[NotificationType.GOVERNANCE_PROPOSAL] !== undefined) {
+ dbPrefs.governanceProposal = preferences[NotificationType.GOVERNANCE_PROPOSAL];
+ }
+
+ await prisma.notificationPreference.upsert({
+ where: { userId },
+ update: dbPrefs,
+ create: { userId, ...dbPrefs },
+ });
+
+ console.log(`Updated preferences for user ${userId}:`, preferences);
this.emit("preferences:updated", { userId, preferences });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/services/notification.service.ts` around lines 385 - 392, The
updatePreferences method currently only logs and emits an event; modify
NotificationService.updatePreferences to persist the provided
Partial<Record<NotificationType, boolean>> into the NotificationPreference table
(upserting or creating one row per NotificationType for the given userId) before
emitting the "preferences:updated" event; ensure you map missing keys to their
current values or defaults, perform the DB writes via the existing
NotificationPreference repository/ORM within a transaction, handle and surface
DB errors (rejecting the promise) and only emit the event after successful
persistence.
Owner
|
Fix failing CI, please |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/services/loan.service.ts (1)
419-479:⚠️ Potential issue | 🔴 CriticalRemove the accidentally duplicated
recordRepayment()tail.The code on lines 419–468 repeats logic from the transaction body above it and incorrectly closes
db.$transaction()/recordRepayment()early. Thereturnstatement that follows on line 470 is then parsed at class scope instead of within the method, producing the TS1068 compilation error. Additionally, this duplicated tail would rebroadcast the status update and droppaymentSessionfrom the result that the code below (lines 488–496) consumes. Consolidate to a single transaction tail and fold the notification logic into that path.Minimal cleanup to get back to a single transaction tail
- const outstandingAfter = outstandingBefore.minus(amount); - let nextStatus: LoanStatus = loan.status; - if (outstandingAfter.eq(ZERO)) { - nextStatus = "REPAID"; - } else if (loan.status === "PENDING") { - nextStatus = "ACTIVE"; - } - - if (nextStatus !== loan.status) { - await tx.loan.update({ - where: { id: loanId }, - data: { status: nextStatus }, - }); - - websocketService.broadcastLoanUpdated(loanId, nextStatus); - - // Send notification if loan is approved (moved to ACTIVE) - if (nextStatus === "ACTIVE") { - await notificationService.sendNotification({ - userId: loan.borrowerId, - type: NotificationType.LOAN_APPROVED, - channel: NotificationChannel.BOTH, - data: { - loanId, - amount: loan.amount.toString(), - assetCode: loan.assetCode, - interestRate: loan.interestRate.toString(), - dueDate: loan.dueDate?.toISOString() || "N/A", - dashboardUrl: `${process.env.FRONTEND_URL || "https://stellovault.com"}/loans/${loanId}`, - }, - }); - } - } - - const updatedLoan = await tx.loan.findUnique({ - where: { id: loanId }, - include: { repayments: true, borrower: true, lender: true }, - }); - if (!updatedLoan) { - throw new NotFoundError("Loan not found"); - } - - return { - repayment, - outstandingBefore: outstandingBefore.toString(), - outstandingAfter: outstandingAfter.toString(), - fullyRepaid: outstandingAfter.eq(ZERO), - loan: updatedLoan, - }; - }, { isolationLevel: "Serializable" }); - } return { repayment, paymentSession: selectedPaymentSession, outstandingBefore: outstandingBefore.toString(), outstandingAfter: outstandingAfter.toString(), fullyRepaid: outstandingAfter.eq(ZERO), loan: updatedLoan, }; }, { isolationLevel: "Serializable" }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/services/loan.service.ts` around lines 419 - 479, The duplicated transaction tail after recordRepayment() must be removed and consolidated into the single db.$transaction() body: eliminate the extra closing of db.$transaction()/recordRepayment() and the duplicate block that re-computes outstandingAfter, nextStatus, updates the loan, calls websocketService.broadcastLoanUpdated, and sends notification via notificationService.sendNotification; instead keep that logic inside the original recordRepayment/db.$transaction flow (references: recordRepayment, db.$transaction, websocketService.broadcastLoanUpdated, notificationService.sendNotification) and ensure the final return inside the transaction returns repayment, paymentSession, outstandingBefore/outstandingAfter strings, fullyRepaid, and the updatedLoan so the outer method no longer contains a stray return or duplicated logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/.env.example`:
- Around line 6-7: Remove the quote characters around the new env values and
reorder the keys to match the file's expected alphabetical order to satisfy
dotenv-linter: change SENDGRID_API_KEY="" and
FROM_EMAIL="noreply@stellovault.com" to unquoted values (empty values may be
left blank as SENDGRID_API_KEY= and FROM_EMAIL=noreply@stellovault.com) and
place them in the proper alphabetical position among the other keys so the
linter no longer reports QuoteCharacter or UnorderedKey warnings.
---
Outside diff comments:
In `@server/src/services/loan.service.ts`:
- Around line 419-479: The duplicated transaction tail after recordRepayment()
must be removed and consolidated into the single db.$transaction() body:
eliminate the extra closing of db.$transaction()/recordRepayment() and the
duplicate block that re-computes outstandingAfter, nextStatus, updates the loan,
calls websocketService.broadcastLoanUpdated, and sends notification via
notificationService.sendNotification; instead keep that logic inside the
original recordRepayment/db.$transaction flow (references: recordRepayment,
db.$transaction, websocketService.broadcastLoanUpdated,
notificationService.sendNotification) and ensure the final return inside the
transaction returns repayment, paymentSession,
outstandingBefore/outstandingAfter strings, fullyRepaid, and the updatedLoan so
the outer method no longer contains a stray return or duplicated logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c404beb-da78-400d-997c-a8be131d6f5c
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
server/.env.exampleserver/package.jsonserver/prisma/schema.prismaserver/src/services/loan.service.ts
✅ Files skipped from review due to trivial changes (2)
- server/package.json
- server/prisma/schema.prisma
Comment on lines
+6
to
+7
| SENDGRID_API_KEY="" | ||
| FROM_EMAIL="noreply@stellovault.com" |
There was a problem hiding this comment.
Fix dotenv-linter warnings on new env keys (likely CI blocker).
Line 6 and Line 7 introduce QuoteCharacter warnings, and key order triggers UnorderedKey. This can keep CI red.
Proposed fix
- SENDGRID_API_KEY=""
- FROM_EMAIL="noreply@stellovault.com"
+ FROM_EMAIL=noreply@stellovault.com
+ SENDGRID_API_KEY=📝 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.
Suggested change
| SENDGRID_API_KEY="" | |
| FROM_EMAIL="noreply@stellovault.com" | |
| FROM_EMAIL=noreply@stellovault.com | |
| SENDGRID_API_KEY= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 7-7: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 7-7: [UnorderedKey] The FROM_EMAIL key should go before the SENDGRID_API_KEY key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/.env.example` around lines 6 - 7, Remove the quote characters around
the new env values and reorder the keys to match the file's expected
alphabetical order to satisfy dotenv-linter: change SENDGRID_API_KEY="" and
FROM_EMAIL="noreply@stellovault.com" to unquoted values (empty values may be
left blank as SENDGRID_API_KEY= and FROM_EMAIL=noreply@stellovault.com) and
place them in the proper alphabetical position among the other keys so the
linter no longer reports QuoteCharacter or UnorderedKey warnings.
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
2 participants
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.
Closes #142
Summary by CodeRabbit
New Features
Behavioral Changes