feat: Added support for different email service provider#1512
feat: Added support for different email service provider#1512pranalidhanavade merged 7 commits intomainfrom
Conversation
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
|
Warning Rate limit exceeded@pranalidhanavade has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR centralizes email sending by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant AppSvc as App Service (e.g., UserService)
participant EmailSvc as EmailService
participant SG as SendGrid Helper
participant Res as Resend Helper
AppSvc->>EmailSvc: sendEmail(emailDto)
EmailSvc->>EmailSvc: read EMAIL_PROVIDER or DEFAULT
alt provider == "sendgrid"
EmailSvc->>SG: sendWithSendGrid(emailDto)
SG-->>EmailSvc: Promise<boolean>
else provider == "resend"
EmailSvc->>Res: sendWithResend(emailDto)
Res-->>EmailSvc: Promise<boolean>
else default (sendgrid)
EmailSvc->>SG: sendWithSendGrid(emailDto)
SG-->>EmailSvc: Promise<boolean>
end
EmailSvc-->>AppSvc: Promise<boolean>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 review |
✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
libs/common/src/resend-helper-file.ts (2)
1-7: Consider removing module-leveldotenv.config()call.NestJS applications typically use
@nestjs/configConfigModule for environment variable management. Callingdotenv.config()at the module level may conflict with existing configuration or be redundant.Consider removing the dotenv import and config call, relying instead on the application's existing ConfigModule setup:
-import * as dotenv from 'dotenv'; - import { EmailDto } from './dtos/email.dto'; import { Logger } from '@nestjs/common'; import { Resend } from 'resend'; - -dotenv.config();
22-26: Consider propagating errors instead of swallowing them.The function catches all errors and returns
false, which makes it difficult to diagnose email sending failures. While the error is logged, the calling code loses context about what went wrong.Consider letting errors propagate to the caller for better error handling:
try { const response = await resend.emails.send({ from: emailDto.emailFrom, to: emailDto.emailTo, subject: emailDto.emailSubject, text: emailDto.emailText, html: emailDto.emailHtml, attachments: emailDto.emailAttachments }); return Boolean(response.data?.id); } catch (error) { Logger.error('Error while sending email with Resend', error); - return false; + throw error; }Alternatively, if the current pattern is intentional for graceful degradation, document this behavior in the function's JSDoc.
libs/common/src/smtp-helper-file.ts (2)
1-7: Consider removing module-leveldotenv.config()call.Similar to the Resend helper, this file calls
dotenv.config()at the module level, which may conflict with NestJS's ConfigModule configuration.Remove the dotenv import and config call:
-import * as dotenv from 'dotenv'; import * as nodemailer from 'nodemailer'; import { EmailDto } from './dtos/email.dto'; import { Logger } from '@nestjs/common'; - -dotenv.config();
17-32: Simplify error handling and consider propagating errors.The current implementation mixes try/catch with
.then().catch(), which is redundant. Additionally, like the Resend helper, this function swallows errors by returningfalse.Simplify the error handling:
try { - return await transporter - .sendMail({ + await transporter.sendMail({ from: emailDto.emailFrom, to: emailDto.emailTo, subject: emailDto.emailSubject, text: emailDto.emailText, html: emailDto.emailHtml, attachments: emailDto.emailAttachments - }) - .then(() => true) - .catch(() => false); + }); + return true; } catch (error) { Logger.error('Error while sending email with SMTP', error); - return false; + throw error; }If returning
falseon error is intentional, document this behavior in a JSDoc comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/issuance/src/issuance.service.ts(3 hunks)apps/organization/src/organization.service.ts(4 hunks)apps/user/src/user.service.ts(4 hunks)apps/verification/src/verification.service.ts(3 hunks)libs/common/src/aws-ses-helper-file.ts(1 hunks)libs/common/src/common.module.ts(1 hunks)libs/common/src/email.service.ts(1 hunks)libs/common/src/resend-helper-file.ts(1 hunks)libs/common/src/send-grid-helper-file.ts(2 hunks)libs/common/src/smtp-helper-file.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
libs/common/src/email.service.ts (5)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)libs/common/src/aws-ses-helper-file.ts (1)
sendWithSES(10-39)libs/common/src/send-grid-helper-file.ts (1)
sendWithSendGrid(10-27)libs/common/src/smtp-helper-file.ts (1)
sendWithSMTP(9-33)libs/common/src/resend-helper-file.ts (1)
sendWithResend(9-27)
libs/common/src/resend-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
libs/common/src/smtp-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
libs/common/src/aws-ses-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
libs/common/src/send-grid-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
🔇 Additional comments (4)
apps/issuance/src/issuance.service.ts (2)
95-95: LGTM: EmailService properly injected.The EmailService is correctly imported and injected following NestJS dependency injection patterns. This aligns with the PR's goal of centralizing email sending logic.
Also applies to: 116-118
1093-1093: LGTM: Email sending correctly delegated to EmailService.The email sending logic has been successfully migrated to use the centralized EmailService. The change is clean and maintains existing error handling.
apps/verification/src/verification.service.ts (2)
56-56: LGTM: EmailService properly integrated.The EmailService is correctly imported and injected, following the same pattern used in the issuance service. This ensures consistency across the codebase.
Also applies to: 72-74
665-665: LGTM: Email sending correctly migrated to EmailService.The email sending logic has been successfully updated to use the centralized EmailService, maintaining existing error handling patterns.
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/common/src/aws-ses-helper-file.ts (1)
24-31: Fix attachment format conversion and verify HTML sanitization.Two critical concerns:
Attachment format mismatch (still unresolved from previous review):
emailAttachmentsuses SendGrid'sAttachmentJSONformat (base64content,type,content_id), but nodemailer expects a different structure (encoding: 'base64',contentType,cid). Passing SendGrid attachments directly causes nodemailer to treat base64 as UTF-8 text, corrupting all attachments.HTML injection risk: CodeQL flags
emailHtmlas a potential XSS vector. While email clients provide some protection, verify that HTML content is properly sanitized upstream before being passed to this function, especially if it contains user-provided data.Apply this diff to fix the attachment mapping:
+ const attachments = emailDto.emailAttachments?.map((att) => ({ + filename: att.filename, + content: att.content, + contentType: att.type, + contentDisposition: att.disposition, + cid: att.content_id, + encoding: 'base64' + })); + await transporter.sendMail({ from: emailDto.emailFrom, to: emailDto.emailTo, subject: emailDto.emailSubject, text: emailDto.emailText, html: emailDto.emailHtml, - attachments: emailDto.emailAttachments + attachments });For the HTML injection concern, verify how
emailHtmlis constructed in the calling code to ensure user input is properly escaped or sanitized.
🧹 Nitpick comments (3)
libs/common/src/aws-ses-helper-file.ts (3)
9-9: Consider using NestJS ConfigModule instead of module-level dotenv.Calling
dotenv.config()at the module level introduces side effects and bypasses NestJS's standard ConfigModule pattern. In a NestJS application, environment variables should typically be managed through the ConfigModule, allowing for better testability and configuration management.
12-21: Consider reusing the SES client and transporter for better performance.The SES client and nodemailer transporter are recreated on every
sendWithSEScall. For high-volume email sending, this adds unnecessary overhead. Consider initializing them once at the module level (or lazy-initializing and caching them) and reusing them across calls.Example approach:
let cachedTransporter: nodemailer.Transporter | null = null; const getTransporter = () => { if (!cachedTransporter) { const sesClient = new SESClient({ region: process.env.AWS_SES_REGION, credentials: { accessKeyId: process.env.AWS_SES_ACCESS_KEY, secretAccessKey: process.env.AWS_SES_SECRET_KEY } }); cachedTransporter = nodemailer.createTransport({ SES: { ses: sesClient, aws: { SendRawEmailCommand } } }); } return cachedTransporter; }; export const sendWithSES = async (emailDto: EmailDto): Promise<boolean> => { const transporter = getTransporter(); // ... rest of the function };
33-36: Consider enriching error handling for better observability.Returning
falseon error discards all failure context. The caller cannot distinguish between configuration issues, network failures, SES quota errors, or invalid email addresses. Consider either throwing the error (with added context) or returning a more detailed result object that includes error information.Example approach:
export const sendWithSES = async (emailDto: EmailDto): Promise<{ success: boolean; error?: string }> => { try { // ... sending logic return { success: true }; } catch (error) { Logger.error('Error while sending email with Amazon SES', error); return { success: false, error: error instanceof Error ? error.message : 'Unknown error' }; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/common/src/aws-ses-helper-file.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/common/src/aws-ses-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
🪛 GitHub Check: CodeQL
libs/common/src/aws-ses-helper-file.ts
[failure] 29-29: Client-side cross-site scripting
HTML injection vulnerability due to user-provided value.
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/common/src/resend-helper-file.ts (2)
7-7: Consider movingdotenv.config()to application startup.Calling
dotenv.config()at the module level can lead to redundant environment loading when multiple modules import this file. In NestJS applications, it's typically better to call this once at application startup (e.g., inmain.ts).If this helper needs to work standalone or in tests, the current approach is acceptable. Otherwise, consider removing this line and ensuring environment variables are loaded at application initialization.
9-13: Use the validatedapiKeyvariable.Line 13 re-reads
process.env.RESEND_API_KEYinstead of using theapiKeyvariable that was already validated on lines 9-11. This is a minor redundancy.Apply this diff to use the validated variable:
const apiKey = process.env.RESEND_API_KEY; if (!apiKey) { throw new Error('Missing RESEND_API_KEY in environment variables.'); } -const resend = new Resend(process.env.RESEND_API_KEY); +const resend = new Resend(apiKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/common/src/common.constant.ts(1 hunks)libs/common/src/email.service.ts(1 hunks)libs/common/src/resend-helper-file.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/common/src/email.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/common/src/resend-helper-file.ts (1)
libs/common/src/dtos/email.dto.ts (1)
EmailDto(1-8)
🔇 Additional comments (2)
libs/common/src/common.constant.ts (1)
284-284: Verify the default provider choice aligns with requirements.The default email provider is hardcoded as
'sendgrid'. Since this PR adds support for multiple providers (Resend, AWS-SES, SMTP), ensure that SendGrid as the default is intentional and documented, especially for existing installations that might expect a different default or need explicit configuration.libs/common/src/resend-helper-file.ts (1)
15-31: LGTM!The function correctly implements the Resend email sending flow:
- Uses the singleton client instance
- Maps EmailDto fields appropriately to the Resend API
- Handles success/failure with proper error logging
- Returns a boolean indicating success
The past review concerns about client instantiation and environment validation have been properly addressed.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.demo(1 hunks).env.sample(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.sample
[warning] 226-226: [TrailingWhitespace] Trailing whitespace detected
(TrailingWhitespace)
[warning] 241-241: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
.env.demo
[warning] 207-207: [TrailingWhitespace] Trailing whitespace detected
(TrailingWhitespace)
[warning] 222-222: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (2)
.env.demo (1)
203-222: Email provider configuration is well-structured.The new email provider configuration block is clear and maintainable:
- Active provider (resend) is uncommented and documented
- Fallback options (sendgrid, ses, smtp) are commented out for easy activation
- Provider enum options are explicitly listed in the comment
- Each provider section is properly documented
This aligns well with the multi-provider email service architecture described in the PR objectives.
.env.sample (1)
222-241: Email provider configuration is consistent and well-documented.The email provider configuration in .env.sample mirrors that in .env.demo with the same structure:
- Clear enum of provider options: resend, sendgrid, ses, smtp
- Default provider (resend) is active and documented
- Fallback providers are commented out for easy reference
- Each section includes helpful inline comments
The consistency between .env.demo and .env.sample is good for developer experience.
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
Signed-off-by: pranalidhanavade <pranali.dhanavade@ayanworks.com>
|
4 participants
What
Summary by CodeRabbit
Refactor
New Features
Chores
Impact: Email flows remain functional with improved provider flexibility and centralized handling.