Deep logging for send notification API#50
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds traceId propagation across the notification system, updates adapters and service methods to accept and log traceId, enhances LoggerUtil to accept optional metadata in logs, and introduces maskEmail/maskPhone utilities for masking sensitive data in log output. Changes
Sequence DiagramsequenceDiagram
participant Client
participant NotificationService
participant Adapters as Email/SMS/WhatsApp
participant Logger
participant External as ExternalServices
Client->>NotificationService: send(notificationDataArray)
activate NotificationService
Note over NotificationService: generate traceId (UUID)
NotificationService->>Logger: log("RECEIVED", { traceId })
NotificationService->>NotificationService: validate & prepare (traceId)
NotificationService->>Logger: log("VALIDATED", { traceId })
NotificationService->>Adapters: sendNotification(data, traceId)
deactivate NotificationService
activate Adapters
Adapters->>Logger: log("ADAPTER_PREP", { traceId, metadata: maskedData })
Adapters->>External: send(maskedPayload, traceId)
alt Success
External-->>Adapters: response
Adapters->>Logger: log("SENT", { traceId, messageId, timeTaken })
Adapters-->>NotificationService: result({traceId, status, messageId})
else Failure
External-->>Adapters: error
Adapters->>Logger: log("FAILED", { traceId, error: error.message, timeTaken })
Adapters-->>NotificationService: result({traceId, status: "FAILED", error: error.message})
end
deactivate Adapters
activate NotificationService
NotificationService->>Logger: log("COMPLETED/FAILED", { traceId, summary })
NotificationService-->>Client: APIResponse({ traceId, status })
deactivate NotificationService
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/modules/notification/adapters/whatsappViaGupshup.adapter.ts (1)
151-163:⚠️ Potential issue | 🟠 MajorDon't write the raw Gupshup API key and recipient data to logs.
Line 163 spreads
templateDatastraight into theADAPTER_PREPmetadata. That includesgupshupApiKey, the full WhatsApp number, and raw template params, so every request now leaks secrets/PII into the application logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/adapters/whatsappViaGupshup.adapter.ts` around lines 151 - 163, The ADAPTER_PREP log in sendTemplateMessage currently spreads templateData (including gupshupApiKey, recipient number and raw templateParams) which leaks secrets/PII; update the logging call to only include non-sensitive fields or a sanitized copy: build a sanitizedMetadata object (e.g., include templateId, masked to (last 4 digits) or omit to, masked apiKey or omit gupshupApiKey, and a redacted/summary templateParams) and pass that to LoggerUtil.log instead of spreading templateData; also ensure createRawNotificationLog does not record raw PII by using the sanitized recipient or an identifier returned from that sanitized object.src/modules/notification/notification.service.ts (2)
464-476:⚠️ Potential issue | 🔴 CriticalRemove spaces from FCM topic path and authorization header.
Line 471 sends
to:/ topics / ${topic_name}and line 476 sends `Authorization: `key = ${fcmKey}. The embedded spaces create invalid FCM API values: the topic path becomes/ topics / topicnameinstead of/topics/topicname, and the auth header becomeskey = valueinstead ofkey=value. These will cause FCM requests to fail.Fix
- to: `/ topics / ${topic_name} `, + to: `/topics/${topic_name}`,- Authorization: `key = ${fcmKey} `, + Authorization: `key=${fcmKey}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/notification.service.ts` around lines 464 - 476, The FCM topic path and Authorization header include stray spaces causing invalid values; update the notification payload and headers where notificationData and the axios.post call are assembled (look for notificationData and the headers block using fcmUrl/fcmKey) to use the correct strings: set to: `/topics/${topic_name}` (no spaces) and Authorization: `key=${fcmKey}` (no spaces around =), ensuring the template literals produce `/topics/topicname` and `key=yourKey`.
358-370:⚠️ Potential issue | 🔴 CriticalFix the
AmqpConnection.publish()call—arguments are in wrong order.Line 366 passes arguments as
publish(apiId, "notification.exchange", "notification.route", result, ...)but the correct signature ispublish(exchange, routingKey, message, options). The call should be:this.amqpConnection.publish( "notification.exchange", "notification.route", result, { persistent: true } );Remove the
apiIdargument entirely. Additionally, the subscriber handler parameter names at line 390 are misleading—the first parameter receives the message payload (not a traceId), the second receives the AMQP message object (not a notification), and the third receives headers. Consider using@RabbitPayload(),@RabbitRequest(), and@RabbitHeader()decorators for clarity, or rename the handler parameters to match what they actually receive. The retry call at line 407 also passes arguments in the wrong order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/notification.service.ts` around lines 358 - 370, In saveNotificationQueue fix the Amqp publish call by removing the extraneous apiId argument and call amqpConnection.publish with the correct signature: publish("notification.exchange", "notification.route", result, { persistent: true }) (see the method saveNotificationQueue and use amqpConnection.publish). Update the subscriber handler parameter names (or switch to `@RabbitPayload`(), `@RabbitRequest`(), `@RabbitHeader`() decorators) so the first param represents payload, second the AMQP message/request, third headers (rename misleading variables used in the subscriber method). Also correct the retry call (near the retry usage in this file) to pass arguments in the proper order expected by that retry function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/notification/adapters/emailService.adapter.ts`:
- Around line 18-24: The validator incorrectly calls
isValidEmail(singleEmailData.to) while RawEmailData.to can be string | string[];
update the validation in sendRawEmail (or the surrounding method handling
singleEmailData) to accept arrays by checking if singleEmailData.to is an array
and, if so, iterate and call isValidEmail on each entry (failing if any are
invalid), otherwise call isValidEmail on the single string; ensure this change
is applied to the same pattern at the other occurrences noted (lines around
113–115) so multi-recipient inputs pass validation.
In `@src/modules/notification/adapters/smsService.adapter.ts`:
- Around line 267-281: The ADAPTER_PREP log is leaking PII by logging
singleSmsData verbatim and passing the literal 'info' as the context; update the
LoggerUtil.log call in the loop that validates singleSmsData (referencing
smsProvider, SMS_PROVIDER.MSG_91, singleSmsData, traceId) to instead build a
sanitized payload that masks phone numbers and redacts message
bodies/replacement values (e.g., replace with "[REDACTED]" or masked OTP) and
only include non-sensitive metadata (templateId, provider, status); also pass
the proper trace context (traceId or existing trace object) into the context
parameter instead of the string 'info' so entries can be correlated with the
trace.
- Around line 287-306: The success check treats only AWS/MSG91 responses as
success, so Twilio sends (which return a message object with `sid`) are
misclassified as failures; update the condition around `result` in the SMS send
handling (the block that logs via `LoggerUtil.log` and pushes into `results`) to
also treat `result?.sid` as success (i.e., success when
`$metadata.httpStatusCode === 200 && MessageId` OR `result?.sid`), ensure the
success branch uses `SUCCESS_MESSAGES.SMS_NOTIFICATION_SEND_SUCCESSFULLY` and
includes `messageId` using `result.MessageId || result.sid ||
\`sms-${Date.now()}\``, and keep the failure branch’s safe stringification for
non-serializable fields unchanged.
In `@src/modules/notification/notification.service.ts`:
- Around line 771-779: maskEmail currently computes '*'.repeat(name.length - 4)
which throws for short local-parts; update maskEmail to handle short names by
branching: if name.length > 4 keep the existing pattern (first 2 +
repeat(name.length-4) + last 2), otherwise construct a safe maskedName that
reveals only minimal characters (e.g., for length 1 return '*', for length 2
return first char + '*', for length 3+4 reveal first and last with middle
replaced by stars) so no negative repeat/counts are used; adjust the maskedName
construction around the name variable and ensure the domain join remains the
same.
---
Outside diff comments:
In `@src/modules/notification/adapters/whatsappViaGupshup.adapter.ts`:
- Around line 151-163: The ADAPTER_PREP log in sendTemplateMessage currently
spreads templateData (including gupshupApiKey, recipient number and raw
templateParams) which leaks secrets/PII; update the logging call to only include
non-sensitive fields or a sanitized copy: build a sanitizedMetadata object
(e.g., include templateId, masked to (last 4 digits) or omit to, masked apiKey
or omit gupshupApiKey, and a redacted/summary templateParams) and pass that to
LoggerUtil.log instead of spreading templateData; also ensure
createRawNotificationLog does not record raw PII by using the sanitized
recipient or an identifier returned from that sanitized object.
In `@src/modules/notification/notification.service.ts`:
- Around line 464-476: The FCM topic path and Authorization header include stray
spaces causing invalid values; update the notification payload and headers where
notificationData and the axios.post call are assembled (look for
notificationData and the headers block using fcmUrl/fcmKey) to use the correct
strings: set to: `/topics/${topic_name}` (no spaces) and Authorization:
`key=${fcmKey}` (no spaces around =), ensuring the template literals produce
`/topics/topicname` and `key=yourKey`.
- Around line 358-370: In saveNotificationQueue fix the Amqp publish call by
removing the extraneous apiId argument and call amqpConnection.publish with the
correct signature: publish("notification.exchange", "notification.route",
result, { persistent: true }) (see the method saveNotificationQueue and use
amqpConnection.publish). Update the subscriber handler parameter names (or
switch to `@RabbitPayload`(), `@RabbitRequest`(), `@RabbitHeader`() decorators) so the
first param represents payload, second the AMQP message/request, third headers
(rename misleading variables used in the subscriber method). Also correct the
retry call (near the retry usage in this file) to pass arguments in the proper
order expected by that retry function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03746494-6928-49da-8222-73ee5eecd6cd
📒 Files selected for processing (7)
src/common/logger/LoggerUtil.tssrc/modules/notification/adapters/emailService.adapter.tssrc/modules/notification/adapters/smsService.adapter.tssrc/modules/notification/adapters/whatsappViaGupshup.adapter.tssrc/modules/notification/interface/notificationService.tssrc/modules/notification/notification.controller.tssrc/modules/notification/notification.service.ts
| to: string | string[]; | ||
| subject: string; | ||
| body: string; | ||
| from?: string; | ||
| isHtml?: boolean; | ||
| cc?: string[]; | ||
| bcc?: string[]; |
There was a problem hiding this comment.
RawEmailData.to now allows arrays, but this validator still rejects them.
Line 18 advertises string | string[], yet Line 113 passes singleEmailData.to straight into isValidEmail(). Any caller that uses the new multi-recipient shape will fail validation before sendRawEmail() even though the rest of this method already handles arrays.
🛠️ Proposed fix
- if (!singleEmailData.to || !this.isValidEmail(singleEmailData.to)) {
+ const recipients = Array.isArray(singleEmailData.to)
+ ? singleEmailData.to
+ : [singleEmailData.to];
+
+ if (!recipients.length || recipients.some((email) => !this.isValidEmail(email))) {
throw new BadRequestException(ERROR_MESSAGES.INVALID_EMAIL);
}Also applies to: 113-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/adapters/emailService.adapter.ts` around lines 18 -
24, The validator incorrectly calls isValidEmail(singleEmailData.to) while
RawEmailData.to can be string | string[]; update the validation in sendRawEmail
(or the surrounding method handling singleEmailData) to accept arrays by
checking if singleEmailData.to is an array and, if so, iterate and call
isValidEmail on each entry (failing if any are invalid), otherwise call
isValidEmail on the single string; ensure this change is applied to the same
pattern at the other occurrences noted (lines around 113–115) so multi-recipient
inputs pass validation.
| for (const singleSmsData of smsDataArray) { | ||
| try { | ||
| // Validate based on provider | ||
| if (this.smsProvider === SMS_PROVIDER.MSG_91) { | ||
| // For MSG91, templateId is required, body is optional | ||
| if (!singleSmsData.templateId) { | ||
| throw new BadRequestException("templateId is required for MSG91"); | ||
| } | ||
| } else { | ||
| // For other providers, body is required | ||
| if (!singleSmsData.body) { | ||
| throw new BadRequestException("SMS body is required"); | ||
| } | ||
| try { | ||
| // Validate based on provider | ||
| if (this.smsProvider === SMS_PROVIDER.MSG_91) { | ||
| // For MSG91, templateId is required, body is optional | ||
| if (!singleSmsData.templateId) { | ||
| throw new BadRequestException("templateId is required for MSG91"); | ||
| } | ||
| } else { | ||
| // For other providers, body is required | ||
| if (!singleSmsData.body) { | ||
| throw new BadRequestException("SMS body is required"); | ||
| } | ||
| } | ||
| LoggerUtil.log(`ADAPTER_PREP`, 'info', undefined, 'info', { ...singleSmsData, traceId: traceId, status: 'ADAPTER_PREP' }); |
There was a problem hiding this comment.
Raw SMS prep logs are leaking recipient/body data.
Line 281 logs singleSmsData verbatim, so this path now writes full phone numbers, message bodies, and replacement values like OTPs to the log stream. It also passes 'info' into the context slot instead of the trace context, which makes these entries harder to correlate with the rest of the trace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/adapters/smsService.adapter.ts` around lines 267 -
281, The ADAPTER_PREP log is leaking PII by logging singleSmsData verbatim and
passing the literal 'info' as the context; update the LoggerUtil.log call in the
loop that validates singleSmsData (referencing smsProvider, SMS_PROVIDER.MSG_91,
singleSmsData, traceId) to instead build a sanitized payload that masks phone
numbers and redacts message bodies/replacement values (e.g., replace with
"[REDACTED]" or masked OTP) and only include non-sensitive metadata (templateId,
provider, status); also pass the proper trace context (traceId or existing trace
object) into the context parameter instead of the string 'info' so entries can
be correlated with the trace.
| if (result?.$metadata?.httpStatusCode === 200 && result?.MessageId) { | ||
| LoggerUtil.log(`SENT`, 'info', undefined, 'info', { ...result, traceId: traceId, status: 'SENT', timeTaken: timeTakenInMs }); | ||
| results.push({ | ||
| to: singleSmsData.to, | ||
| status: 200, | ||
| result: SUCCESS_MESSAGES.SMS_NOTIFICATION_SEND_SUCCESSFULLY, | ||
| messageId: result.MessageId || `sms-${Date.now()}` | ||
| }); | ||
| } else { | ||
| LoggerUtil.log(`FAILED`, 'info', undefined, 'info', { ...result, traceId: traceId, status: 'FAILED', timeTaken: timeTakenInMs }); | ||
| // Safe stringification - only include serializable data | ||
| const safeResult = { | ||
| data: result?.data, | ||
| status: result?.status | ||
| }; | ||
| results.push({ | ||
| to: singleSmsData.to, | ||
| status: 400, | ||
| error: `SMS not sent: ${JSON.stringify(safeResult)}` | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/modules/notification/adapters/smsService.adapter.ts | head -320 | tail -50Repository: tekdi/notification-microservice
Length of output: 3033
🏁 Script executed:
grep -n "sendRawSms\|sendViaTwilio\|sendViaSNS\|sendViaMsg91" src/modules/notification/adapters/smsService.adapter.tsRepository: tekdi/notification-microservice
Length of output: 717
🏁 Script executed:
sed -n '186,217p' src/modules/notification/adapters/smsService.adapter.tsRepository: tekdi/notification-microservice
Length of output: 1106
🏁 Script executed:
sed -n '218,260p' src/modules/notification/adapters/smsService.adapter.tsRepository: tekdi/notification-microservice
Length of output: 1953
🏁 Script executed:
sed -n '325,365p' src/modules/notification/adapters/smsService.adapter.tsRepository: tekdi/notification-microservice
Length of output: 1620
🏁 Script executed:
sed -n '365,400p' src/modules/notification/adapters/smsService.adapter.tsRepository: tekdi/notification-microservice
Length of output: 1702
🏁 Script executed:
grep -n "SMS_PROVIDER\|twilio" src/modules/notification/adapters/smsService.adapter.ts | head -20Repository: tekdi/notification-microservice
Length of output: 1432
🌐 Web query:
Twilio Node.js SDK message object properties sid
💡 Result:
In the Twilio Node.js helper library (twilio / twilio-node), the object you get back from client.messages.create(...) is a Message instance and it includes a sid property, accessible as message.sid (commonly logged right after creation). [1]
sid is the unique identifier of the Message resource in Twilio’s API. For Messages, the SID format is 34 characters and matches ^(SM|MM)[0-9a-fA-F]{32}$ (typically starts with SM...; MM... can also occur). [2]
Example:
client.messages
.create({ body: "Hello", from: "+1XXX", to: "+1YYY" })
.then(message => console.log(message.sid));[1]
Sources
- Twilio Node.js helper library README (example logging
message.sid) [1] - Twilio Messaging “Message resource” docs (Message SID definition + pattern) [2]
Twilio sends are incorrectly reported as failures.
The success condition at line 287 only checks for $metadata.httpStatusCode === 200 && MessageId, which matches AWS SNS and MSG91 response formats. However, sendViaTwilio() returns a Twilio message object with a sid property but no $metadata or MessageId, causing all successful Twilio SMS sends to incorrectly fall into the failure path instead.
The proposed fix is correct: add a provider-specific check for Twilio's sid property alongside the AWS/MSG91 checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/adapters/smsService.adapter.ts` around lines 287 -
306, The success check treats only AWS/MSG91 responses as success, so Twilio
sends (which return a message object with `sid`) are misclassified as failures;
update the condition around `result` in the SMS send handling (the block that
logs via `LoggerUtil.log` and pushes into `results`) to also treat `result?.sid`
as success (i.e., success when `$metadata.httpStatusCode === 200 && MessageId`
OR `result?.sid`), ensure the success branch uses
`SUCCESS_MESSAGES.SMS_NOTIFICATION_SEND_SUCCESSFULLY` and includes `messageId`
using `result.MessageId || result.sid || \`sms-${Date.now()}\``, and keep the
failure branch’s safe stringification for non-serializable fields unchanged.
| export function maskEmail(email: string) { | ||
| const [name, domain] = email.split("@"); | ||
|
|
||
| const maskedName = | ||
| name.substring(0, 2) + | ||
| "*".repeat(name.length - 4) + | ||
| name.substring(name.length - 2); | ||
|
|
||
| return `${maskedName}@${domain}`; |
There was a problem hiding this comment.
maskEmail() throws for short but valid addresses.
Line 776 calls '*'.repeat(name.length - 4), which becomes negative for valid local-parts like ab@example.com. Because the adapters invoke this helper before sending, those notifications can fail purely because logging tried to mask the recipient.
🛠️ Proposed fix
export function maskEmail(email: string) {
const [name, domain] = email.split("@");
+
+ if (name.length <= 2) {
+ return `${name[0] ?? ""}*@${domain}`;
+ }
+ if (name.length <= 4) {
+ return `${name[0]}${"*".repeat(name.length - 2)}${name.slice(-1)}@${domain}`;
+ }
const maskedName =
name.substring(0, 2) +
"*".repeat(name.length - 4) +
name.substring(name.length - 2);📝 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.
| export function maskEmail(email: string) { | |
| const [name, domain] = email.split("@"); | |
| const maskedName = | |
| name.substring(0, 2) + | |
| "*".repeat(name.length - 4) + | |
| name.substring(name.length - 2); | |
| return `${maskedName}@${domain}`; | |
| export function maskEmail(email: string) { | |
| const [name, domain] = email.split("@"); | |
| if (name.length <= 2) { | |
| return `${name[0] ?? ""}*@${domain}`; | |
| } | |
| if (name.length <= 4) { | |
| return `${name[0]}${"*".repeat(name.length - 2)}${name.slice(-1)}@${domain}`; | |
| } | |
| const maskedName = | |
| name.substring(0, 2) + | |
| "*".repeat(name.length - 4) + | |
| name.substring(name.length - 2); | |
| return `${maskedName}@${domain}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/notification.service.ts` around lines 771 - 779,
maskEmail currently computes '*'.repeat(name.length - 4) which throws for short
local-parts; update maskEmail to handle short names by branching: if name.length
> 4 keep the existing pattern (first 2 + repeat(name.length-4) + last 2),
otherwise construct a safe maskedName that reveals only minimal characters
(e.g., for length 1 return '*', for length 2 return first char + '*', for length
3+4 reveal first and last with middle replaced by stars) so no negative
repeat/counts are used; adjust the maskedName construction around the name
variable and ensure the domain join remains the same.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/notification/notification.service.ts (2)
405-408:⚠️ Potential issue | 🔴 CriticalRecursive call to
handleNotificationdropstraceId, causing parameter misalignment.Line 407 invokes
handleNotification(notification, message, retryCount - 1), but the method signature at Line 390 expects(traceId, notification, message, retryCount). This will passnotificationastraceId,messageasnotification, and so on—breaking retry logic and logging.🐛 Proposed fix
if (retryCount > 0) { setTimeout(async () => { - await this.handleNotification(notification, message, retryCount - 1); + await this.handleNotification(traceId, notification, message, retryCount - 1); }, 2000); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/notification.service.ts` around lines 405 - 408, The retry setTimeout is calling handleNotification with the wrong argument order, causing traceId to be overwritten; update the call inside the timeout to pass the original traceId first (i.e., call handleNotification(traceId, notification, message, retryCount - 1)) and ensure the surrounding scope captures the correct traceId variable used by handleNotification so retries pass the same traceId, notification, message, and decremented retryCount.
358-371:⚠️ Potential issue | 🔴 Critical
saveNotificationQueueparameter renamed but semantically misused.The first parameter is named
apiIdbut receivestraceIdfrom callers (Line 295). Additionally, Line 366 passes this value toamqpConnection.publish()as the first argument, which expects an exchange name—not a trace ID. This will cause messages to be published to an incorrect exchange or fail entirely.🐛 Proposed fix — keep exchange name correct
- async saveNotificationQueue(apiId, notificationDataArray) { + async saveNotificationQueue(traceId: string, notificationDataArray) { const arrayofResult = await this.notificationQueue.save( notificationDataArray ); if (arrayofResult) { if (this.amqpConnection) { try { for (const result of arrayofResult) { - this.amqpConnection.publish(apiId, - "notification.exchange", + this.amqpConnection.publish( + "notification.exchange", "notification.route", result, { persistent: true } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/notification.service.ts` around lines 358 - 371, The first parameter of saveNotificationQueue is actually a traceId but is being passed as the first argument to amqpConnection.publish (which should be the exchange name); update saveNotificationQueue so amqpConnection.publish is called with the correct exchange name ("notification.exchange") as the first argument and routing key ("notification.route") as the second, and send the traceId (rename the parameter to traceId) as part of the message properties (e.g., headers or correlationId) or inside the payload; adjust references to saveNotificationQueue and the local variable name accordingly.
🧹 Nitpick comments (3)
src/modules/notification/adapters/emailService.adapter.ts (2)
315-315: Extraneous space in messageId fallback.Line 315 has
email - ${Date.now()}with spaces around the hyphen, while Line 152 usesemail-${Date.now()}without spaces. This inconsistency could affect log correlation or message ID parsing.🔧 Proposed fix for consistency
- messageId: result.id || `email - ${Date.now()}` + messageId: result.id || `email-${Date.now()}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/adapters/emailService.adapter.ts` at line 315, The messageId fallback in the emailService adapter is inconsistent: update the fallback expression in the assignment (messageId: result.id || `email - ${Date.now()}`) to use the same compact format `email-${Date.now()}` as used elsewhere (e.g., the other messageId creation), so change the template literal to `email-${Date.now()}` in the code around the messageId property in the emailService.adapter.ts.
78-78: LoggerUtil calls passtraceIdascontextparameter.Similar to
notification.service.ts, theLoggerUtil.error()calls passtraceIdwhere a descriptive context string is expected (third parameter). This reduces log searchability by context. Consider using a consistent context like'EmailAdapter'or the method name.Also applies to: 88-88, 155-155, 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/adapters/emailService.adapter.ts` at line 78, LoggerUtil.error calls in this adapter are passing traceId as the context parameter; update those calls (the LoggerUtil.error invocations using traceId/traceId variable along with result.message and timeTakenInMs) to supply a consistent context string like 'EmailAdapter' or the specific method name (e.g., 'sendEmail') as the third argument and keep traceId inside the metadata object (e.g., { status: 'FAILED', traceId, timeTaken: timeTakenInMs }); apply the same change to all similar LoggerUtil.error usages in this file (including the other occurrences noted).src/modules/notification/notification.service.ts (1)
218-219: LoggerUtil calls passtraceIdwherecontextis expected.The
LoggerUtil.log()signature is(message, context?, user?, level, metadata?). PassingtraceIdas the second argument (Lines 218, 219, 292, 301, etc.) places a UUID where a descriptive context string (e.g.,apiIdor method name) belongs. This degrades log filtering/grouping capabilities.Consider using
apiIdor a descriptive context string for thecontextparameter and keepingtraceIdonly in the metadata object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/notification/notification.service.ts` around lines 218 - 219, LoggerUtil.log calls are passing traceId as the second (context) parameter; update each call (e.g., LoggerUtil.log invocations in notification.service.ts) to supply a descriptive context string (like apiId or the method name) as the second argument and move traceId into the metadata object (last argument), e.g., keep level and metadata shape but ensure metadata includes traceId while context is a human-readable identifier to preserve proper log grouping/filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/notification/adapters/emailService.adapter.ts`:
- Line 264: The log currently prints the literal text
"SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY" instead of the
constant's value; update the LoggerUtil.log call in emailService.adapter.ts (the
line using LoggerUtil.log with traceId) to interpolate or concatenate the
SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY constant (e.g., use a
template literal with ${SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY}
or string concatenation) so the actual success message value is logged alongside
traceId.
- Around line 86-88: The error timing is wrong because startTime is never set on
the thrown error; capture a startTime variable before entering the try blocks
and use that to compute timeTakenInMs in the catch instead of error.startTime;
update the functions where this occurs (the failing block and sendRawEmails) to
declare const startTime = Date.now() before their try and compute timeTakenInMs
= Date.now() - startTime in their catch handlers, then pass that timeTakenInMs
to LoggerUtil.error.
In `@src/modules/notification/notification.service.ts`:
- Around line 783-785: maskPhone currently assumes phone has >=4 chars and
produces incorrect/overlapping output for short strings; update maskPhone to
guard on length: if phone.length === 0 return empty string (or if you prefer
keep returning empty), if phone.length <= 4 return a string of '*' repeated
phone.length, otherwise return phone.slice(0,2) + '*'.repeat(Math.max(6,
phone.length - 4)) + phone.slice(-2) so short inputs are fully masked and longer
numbers keep first/last two chars with a sensible masked middle; modify the
maskPhone function accordingly.
---
Outside diff comments:
In `@src/modules/notification/notification.service.ts`:
- Around line 405-408: The retry setTimeout is calling handleNotification with
the wrong argument order, causing traceId to be overwritten; update the call
inside the timeout to pass the original traceId first (i.e., call
handleNotification(traceId, notification, message, retryCount - 1)) and ensure
the surrounding scope captures the correct traceId variable used by
handleNotification so retries pass the same traceId, notification, message, and
decremented retryCount.
- Around line 358-371: The first parameter of saveNotificationQueue is actually
a traceId but is being passed as the first argument to amqpConnection.publish
(which should be the exchange name); update saveNotificationQueue so
amqpConnection.publish is called with the correct exchange name
("notification.exchange") as the first argument and routing key
("notification.route") as the second, and send the traceId (rename the parameter
to traceId) as part of the message properties (e.g., headers or correlationId)
or inside the payload; adjust references to saveNotificationQueue and the local
variable name accordingly.
---
Nitpick comments:
In `@src/modules/notification/adapters/emailService.adapter.ts`:
- Line 315: The messageId fallback in the emailService adapter is inconsistent:
update the fallback expression in the assignment (messageId: result.id || `email
- ${Date.now()}`) to use the same compact format `email-${Date.now()}` as used
elsewhere (e.g., the other messageId creation), so change the template literal
to `email-${Date.now()}` in the code around the messageId property in the
emailService.adapter.ts.
- Line 78: LoggerUtil.error calls in this adapter are passing traceId as the
context parameter; update those calls (the LoggerUtil.error invocations using
traceId/traceId variable along with result.message and timeTakenInMs) to supply
a consistent context string like 'EmailAdapter' or the specific method name
(e.g., 'sendEmail') as the third argument and keep traceId inside the metadata
object (e.g., { status: 'FAILED', traceId, timeTaken: timeTakenInMs }); apply
the same change to all similar LoggerUtil.error usages in this file (including
the other occurrences noted).
In `@src/modules/notification/notification.service.ts`:
- Around line 218-219: LoggerUtil.log calls are passing traceId as the second
(context) parameter; update each call (e.g., LoggerUtil.log invocations in
notification.service.ts) to supply a descriptive context string (like apiId or
the method name) as the second argument and move traceId into the metadata
object (last argument), e.g., keep level and metadata shape but ensure metadata
includes traceId while context is a human-readable identifier to preserve proper
log grouping/filtering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d98fdb39-e270-4b0a-bd82-39ba0251559a
📒 Files selected for processing (3)
src/modules/notification/adapters/emailService.adapter.tssrc/modules/notification/adapters/smsService.adapter.tssrc/modules/notification/notification.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/notification/adapters/smsService.adapter.ts
| catch (error) { | ||
| LoggerUtil.error(ERROR_MESSAGES.EMAIL_NOTIFICATION_FAILED, error); | ||
| const timeTakenInMs = Date.now() - (error.startTime || Date.now()); | ||
| LoggerUtil.error(`FAILED ${traceId}`, error.message, traceId, '', { status: 'FAILED', traceId: traceId, timeTaken: timeTakenInMs }); |
There was a problem hiding this comment.
error.startTime is undefined—timeTakenInMs calculation is incorrect.
Line 87 references error.startTime, which is never set. The fallback Date.now() makes timeTakenInMs effectively zero, providing misleading timing data in error logs. The same issue exists at Line 164.
🛠️ Proposed fix — capture startTime before try block
async sendNotification(notificationDataArray, traceId?: string) {
const results = [];
for (const notificationData of notificationDataArray) {
-
+ const startTime = Date.now();
try {
const recipient = notificationData.recipient;
if (!recipient || !this.isValidEmail(recipient)) {
throw new BadRequestException(ERROR_MESSAGES.INVALID_EMAIL);
}
// ... existing masking code ...
- const startTime = Date.now();
const result = await this.send(notificationData, traceId);
const timeTakenInMs = Date.now() - startTime;
// ... rest of try block ...
}
catch (error) {
- const timeTakenInMs = Date.now() - (error.startTime || Date.now());
+ const timeTakenInMs = Date.now() - startTime;
LoggerUtil.error(`FAILED ${traceId}`, error.message, traceId, '', { status: 'FAILED', traceId: traceId, timeTaken: timeTakenInMs });Apply the same pattern to sendRawEmails (Lines 142, 164).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/adapters/emailService.adapter.ts` around lines 86 -
88, The error timing is wrong because startTime is never set on the thrown
error; capture a startTime variable before entering the try blocks and use that
to compute timeTakenInMs in the catch instead of error.startTime; update the
functions where this occurs (the failing block and sendRawEmails) to declare
const startTime = Date.now() before their try and compute timeTakenInMs =
Date.now() - startTime in their catch handlers, then pass that timeTakenInMs to
LoggerUtil.error.
| notificationLogs.status = true; | ||
| await this.notificationServices.saveNotificationLogs(notificationLogs); | ||
| LoggerUtil.log(SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY); | ||
| LoggerUtil.log(`traceId: ${traceId} SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY`, traceId, "", "info",); |
There was a problem hiding this comment.
Typo in log message—string interpolation includes literal text.
Line 264 logs "traceId: ${traceId} SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY" where SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY is literal text instead of the constant value.
🛠️ Proposed fix
- LoggerUtil.log(`traceId: ${traceId} SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY`, traceId, "", "info",);
+ LoggerUtil.log(`traceId: ${traceId} ${SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY}`, traceId, "", "info");📝 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.
| LoggerUtil.log(`traceId: ${traceId} SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY`, traceId, "", "info",); | |
| LoggerUtil.log(`traceId: ${traceId} ${SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY}`, traceId, "", "info"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/adapters/emailService.adapter.ts` at line 264, The
log currently prints the literal text
"SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY" instead of the
constant's value; update the LoggerUtil.log call in emailService.adapter.ts (the
line using LoggerUtil.log with traceId) to interpolate or concatenate the
SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY constant (e.g., use a
template literal with ${SUCCESS_MESSAGES.EMAIL_NOTIFICATION_SEND_SUCCESSFULLY}
or string concatenation) so the actual success message value is logged alongside
traceId.
| export function maskPhone(phone: string) { | ||
| return phone.slice(0, 2) + "******" + phone.slice(-2); | ||
| } |
There was a problem hiding this comment.
maskPhone() fails or produces incorrect output for short phone numbers.
Similar to maskEmail, maskPhone assumes at least 4 characters. For a phone number like "123", phone.slice(0, 2) + "******" + phone.slice(-2) yields "12******23" (overlapping slices) instead of a proper mask. For empty or very short strings, this produces nonsensical output that could leak data or cause downstream issues.
🛠️ Proposed fix with guard
export function maskPhone(phone: string) {
+ if (!phone || phone.length <= 4) {
+ return phone ? '*'.repeat(phone.length) : '';
+ }
return phone.slice(0, 2) + "******" + phone.slice(-2);
}📝 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.
| export function maskPhone(phone: string) { | |
| return phone.slice(0, 2) + "******" + phone.slice(-2); | |
| } | |
| export function maskPhone(phone: string) { | |
| if (!phone || phone.length <= 4) { | |
| return phone ? '*'.repeat(phone.length) : ''; | |
| } | |
| return phone.slice(0, 2) + "******" + phone.slice(-2); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/notification/notification.service.ts` around lines 783 - 785,
maskPhone currently assumes phone has >=4 chars and produces
incorrect/overlapping output for short strings; update maskPhone to guard on
length: if phone.length === 0 return empty string (or if you prefer keep
returning empty), if phone.length <= 4 return a string of '*' repeated
phone.length, otherwise return phone.slice(0,2) + '*'.repeat(Math.max(6,
phone.length - 4)) + phone.slice(-2) so short inputs are fully masked and longer
numbers keep first/last two chars with a sensible masked middle; modify the
maskPhone function accordingly.
1 participant
No description provided.