Skip to content

Add Support for Alternate Email Transports #1580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Dec 30, 2024

Conversation

erin-allison
Copy link
Contributor

@erin-allison erin-allison commented Dec 25, 2024

Closes #282
Supercedes #313

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works*

Testing

Configured a local install for AWS SES and then SMTP, verifying each configuration properly sends a magic link email.

*I was unable to test Resend, as I do not have an account with them. The logic for Resend specifically, though, did not change, and it follows the same interface as the others, so there shouldn't be any issues.


Changelog

  • Refactor internal-packages/emails to allow addition of alternative email transports (as opposed to just Resend API)
  • Implement SMTP and AWS SES transports

Screenshots

aws-ses
smtp

I have screen recordings of the testing saved, though I won't be sharing them publicly, as credentials (though they should be invalid by now) are visible. I am happy to provide them privately to the team for further validation, if necessary.

💯

--

Breaking Change

This does introduce one small breaking change: currently to use Resend, all that needs to be specified is RESEND_API_KEY. As a result of needing to support varying config options, EMAIL_TRANSPORT=resend would need to be added to maintain the current behavior.

If this is not done, the application will default to the behavior of printing emails to the console, as if no email transport is configured.

This could be worked around by coding a special case for Resend due to existing deployments, but I didn't think it necessary unless was that deemed to be enough of a concern.

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced email transport configuration options, including support for Resend, SMTP, and AWS SES.
    • Introduced a centralized email transport handling system for improved flexibility.
    • Added new mail transport classes for AWS SES, Resend, SMTP, and a null transport for logging.
    • New optional fields for email transport and alerting mechanisms in the environment schema.
  • Documentation

    • Updated self-hosting guide with detailed instructions for email transport setup and configuration.
    • Enhanced clarity and usability of email configuration instructions.
  • Bug Fixes

    • Improved clarity and usability of email configuration instructions.

Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Signed-off-by: Erin Allison <eallison@andrettikarting.com>
It apparently causes issues when transpiled/bundled

Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Copy link
Contributor

coderabbitai bot commented Dec 25, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/webapp/app/services/email.server.ts

Oops! Something went wrong! :(

ESLint: 8.45.0

ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct.

The config "custom" was referenced from the config file in "/.eslintrc.js".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

Walkthrough

This pull request introduces a comprehensive overhaul of the email transport system in the Trigger.dev application. The changes replace the Resend-specific email sending mechanism with a more flexible, modular approach using Nodemailer. The implementation now supports multiple email transport options including Resend, SMTP, and AWS Simple Email Service (SES), with enhanced configuration flexibility through environment variables and a new transport-agnostic email client architecture.

Changes

File Change Summary
.env.example Updated email transport configuration comments and added instructions for Resend, SMTP, and AWS SES
apps/webapp/app/env.server.ts Added new optional environment variables for email transport configuration
apps/webapp/app/services/email.server.ts Introduced buildTransportOptions function for flexible email transport configuration
docs/open-source-self-hosting.mdx Updated documentation with new email transport configuration instructions
internal-packages/emails/package.json Added dependencies for AWS SES and Nodemailer
internal-packages/emails/src/* Created new transport implementations (AWS SES, SMTP, Resend, Null) with standardized interface

Assessment against linked issues

Objective Addressed Explanation
Remove Resend dependency
Support SMTP configuration
Optional SMTP environment variables
Fallback to console logging

Poem

🐰 Hop, hop, email's on the way!
From Resend to SMTP's bright ray
Nodemailer dances, transports sing
Flexible configs now take wing
Code hops free, no limits today! 📧✨

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (8)
internal-packages/emails/src/transports/aws-ses.ts (1)

13-22: Consider configuring the SES client
If you need to specify a region or credentials explicitly, consider passing them to new awsSes.SESClient(). Otherwise, depending on environment defaults might be acceptable.

apps/webapp/app/services/email.server.ts (1)

35-65: Robust transport configuration
The switch statement correctly handles all transport types, and logging clarifies the chosen transport. One suggestion:

  • Confirm environment variables for AWS region or credentials if needed.

Otherwise, this is a clear and extensible solution.

internal-packages/emails/src/index.tsx (1)

47-54: Constructor consistently applies config
Good job defaulting to a null/undefined transport when no type is specified.

internal-packages/emails/src/transports/null.ts (1)

9-10: Unnecessary constructor
You can remove this empty constructor without impacting functionality.

-  constructor(options: NullMailTransportOptions) {
-  }
+  // Remove the constructor entirely for cleanliness
🧰 Tools
🪛 Biome (1.9.4)

[error] 9-10: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)

internal-packages/emails/src/transports/resend.ts (1)

11-16: Instantiating the Resend client
Straightforward approach. Consider validating the apiKey if it is truly required.

.env.example (2)

38-44: Consider adding email address format examples.

The Resend configuration section is well-documented with clear instructions and links. Consider adding example formats for FROM_EMAIL and REPLY_TO_EMAIL to guide users.

 # FROM_EMAIL=
+# FROM_EMAIL=notifications@yourdomain.com
 # REPLY_TO_EMAIL=
+# REPLY_TO_EMAIL=support@yourdomain.com

57-59: Consider adding AWS region configuration example.

The AWS SES configuration is clear but could benefit from an example AWS region configuration.

 # EMAIL_TRANSPORT=aws-ses
 # FROM_EMAIL=
 # REPLY_TO_EMAIL=
+# AWS_REGION=us-east-1  # Optional: Defaults to AWS SDK region resolution
docs/open-source-self-hosting.mdx (1)

303-305: Minor grammar improvement needed.

The sentence structure could be improved for better readability.

-Credentials are to be supplied as with any other program using the AWS SDK.
-In this scenario, you would likely either supply the additional environment variables `AWS_REGION`, `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` or, when running on AWS, use credentials supplied by the EC2 IMDS.
+Credentials can be supplied in two ways:
+1. Set the environment variables `AWS_REGION`, `AWS_ACCESS_KEY_ID`, and `AWS_SECRET_ACCESS_KEY`
+2. When running on AWS, use credentials from EC2 Instance Metadata Service (IMDS)
🧰 Tools
🪛 LanguageTool

[grammar] ~304-~304: Do not use an adverb of manner between a verb and its direct object.
Context: ...he AWS SDK. In this scenario, you would likely either supply the additional environmen...

(MD_RB_DT)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 21a4fab and 7c00e28.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (11)
  • .env.example (1 hunks)
  • apps/webapp/app/env.server.ts (2 hunks)
  • apps/webapp/app/services/email.server.ts (3 hunks)
  • docs/open-source-self-hosting.mdx (1 hunks)
  • internal-packages/emails/package.json (2 hunks)
  • internal-packages/emails/src/index.tsx (4 hunks)
  • internal-packages/emails/src/transports/aws-ses.ts (1 hunks)
  • internal-packages/emails/src/transports/index.ts (1 hunks)
  • internal-packages/emails/src/transports/null.ts (1 hunks)
  • internal-packages/emails/src/transports/resend.ts (1 hunks)
  • internal-packages/emails/src/transports/smtp.ts (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/open-source-self-hosting.mdx

[grammar] ~304-~304: Do not use an adverb of manner between a verb and its direct object.
Context: ...he AWS SDK. In this scenario, you would likely either supply the additional environmen...

(MD_RB_DT)

🪛 Biome (1.9.4)
internal-packages/emails/src/transports/null.ts

[error] 9-10: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)

internal-packages/emails/src/transports/aws-ses.ts

[error] 34-34: Catch clause variable type annotation must be 'any' or 'unknown' if specified.

(parse)


[error] 52-52: Catch clause variable type annotation must be 'any' or 'unknown' if specified.

(parse)

internal-packages/emails/src/transports/smtp.ts

[error] 35-35: Catch clause variable type annotation must be 'any' or 'unknown' if specified.

(parse)


[error] 53-53: Catch clause variable type annotation must be 'any' or 'unknown' if specified.

(parse)

🪛 GitHub Check: typecheck / typecheck
internal-packages/emails/src/transports/aws-ses.ts

[failure] 34-34:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.


[failure] 52-52:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.

internal-packages/emails/src/transports/smtp.ts

[failure] 35-35:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.


[failure] 53-53:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.

🔇 Additional comments (34)
internal-packages/emails/src/transports/aws-ses.ts (3)

1-9: AWS SES Imports & Type Definition Look Good
All imports and the AwsSesMailTransportOptions type appear valid. Nothing critical to address here.


24-33: Ensure consistent error handling
The method correctly logs and rethrows errors as EmailError. This approach is solid for debugging.


42-51: Double-check error handling consistency
Similar to the previous method, keep error handling consistent. Good job logging error details.

apps/webapp/app/services/email.server.ts (2)

3-3: Good import of MailTransportOptions
This ensures typed transport structuring.


Line range hint 17-28: Singleton initialization looks sound
Using the buildTransportOptions function in each singleton ensures the correct transport. No issues here.

internal-packages/emails/src/index.tsx (4)

16-16: Centralized transport construction
Bringing in constructMailTransport fosters a more modular, pluggable design.


41-42: Private transport field
Declaring #transport: MailTransport is a neat way to encapsulate transport details.


65-70: Delegating HTML email sending
Offloading detailed sending logic to the transport is clean and maintainable.


75-79: Consistent plain text email handling
Again, relying on sendPlainText from the transport ensures consistent behavior across all transports.

internal-packages/emails/src/transports/null.ts (3)

1-7: Null transport initialization
This straightforward type indicates no real sending, which is helpful in local or dev scenarios.


12-21: Logging HTML-based message
Sending to the console for local dev is an ideal fallback. Nicely done.


22-29: Logging plain text message
Same concept for plain text—simple and effective for dev usage.

internal-packages/emails/src/transports/resend.ts (3)

1-9: Resend transport configuration
Defining an optional API key addresses user flexibility.


18-33: HTML email sending
Properly logs and rethrows errors. This aligns well with the global error-handling strategy.


35-50: Plain text email sending
Matches the same pattern as HTML-based sending. Good job maintaining consistency.

internal-packages/emails/src/transports/index.ts (7)

1-6: Imports look good.
Importing specific transport classes and React types is organized well. No issues found.


7-13: Ensure consistent property naming.
All fields in MailMessage are consistently named and typed. Looks good for usage as a typed contract.


15-21: Plain text message type is straightforward.
The properties align with the minimal set needed to send a plain-text email.


23-26: Interface-based approach is excellent.
Defining a MailTransport interface provides a clean abstraction layer for different transports.


28-33: Custom error handling is clear and concise.
Using a custom EmailError class differentiates email-specific errors. Good practice.


35-40: Flexible transport options.
Union-based approach for MailTransportOptions fosters a scalable pattern for future transport expansions.


41-52: Defaulting to NullMailTransport is a helpful fallback.
This ensures that if type is undefined, the system degrades gracefully by not sending real emails.

internal-packages/emails/src/transports/smtp.ts (4)

1-3: Import statements are concise.
Using both render (for HTML content) and nodemailer is correctly set up for SMTP.


5-16: SMTP transport options are well-defined.
Optional fields and a clear type property enhance clarity for different SMTP config possibilities.


21-23: Transporter instantiation is correct.
nodemailer.createTransport(options.config) aligns with the recommended usage.


43-52: 🛠️ Refactor suggestion

Similar catch clause type.
Likewise, at line 53:

-    catch (error: Error) {
+    catch (error: unknown) {
     console.error(
       ...
     );
     throw new EmailError(error as Error);
    }

Likely invalid or redundant comment.

apps/webapp/app/env.server.ts (2)

47-56: Expanded email transport fields.
Adding EMAIL_TRANSPORT and SMTP-related fields fosters a flexible, multi-transport environment. Ensure documentation is updated to guide users on which fields must be set to avoid misconfiguration.


205-214: Alert transport configuration mirrors primary email transport.
Having a matching set of alert-specific fields keeps configuration consistent. No issues found.

internal-packages/emails/package.json (1)

12-15: New dependencies support multi-transport architecture.

  • @aws-sdk/client-ses and nodemailer are necessary for SES and SMTP implementations.
  • Type definitions for nodemailer ensure better consistency and maintainability.

Also applies to: 24-24, 31-31

.env.example (2)

34-37: LGTM! Clear introduction to email transport configuration.

The introduction clearly explains the purpose and default behavior of email transport configuration.


45-56: LGTM! Well-documented SMTP configuration.

The SMTP configuration section includes:

  • Clear explanation of STARTTLS usage
  • Common port number pre-filled
  • All necessary configuration options
docs/open-source-self-hosting.mdx (3)

272-275: LGTM! Clear introduction to email configuration.

The introduction effectively explains the default behavior and configuration requirements.


286-289: LGTM! Excellent security explanation for SMTP.

Clear explanation of STARTTLS vs. implicit TLS, helping users make informed security decisions.


306-310: LGTM! Clear AWS SES configuration example.

The configuration example is concise and matches the .env.example format.

Gotta keep the linter happy :)

Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Copy link

changeset-bot bot commented Dec 25, 2024

⚠️ No Changeset found

Latest commit: 98946c2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
internal-packages/emails/src/transports/smtp.ts (1)

5-16: Enhance type safety and security for SMTP configuration.

Consider the following improvements for the configuration type:

  1. Add runtime validation for the port number (valid range: 1-65535)
  2. Document the security implications of the secure option
  3. Consider making auth required when secure is true
 export type SmtpMailTransportOptions = {
   type: "smtp";
   config: {
     host?: string;
-    port?: number;
+    port?: number & { _brand: 'ValidPort' }; // Ensure valid port range at runtime
     secure?: boolean;
+    /** When true, TLS is used. Strongly recommended for production. */
     auth?: {
       user?: string;
       pass?: string;
     };
   };
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c00e28 and 760887f.

📒 Files selected for processing (2)
  • internal-packages/emails/src/transports/aws-ses.ts (1 hunks)
  • internal-packages/emails/src/transports/smtp.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal-packages/emails/src/transports/aws-ses.ts
🔇 Additional comments (1)
internal-packages/emails/src/transports/smtp.ts (1)

18-66: Verify integration with email transport system.

The implementation looks solid but let's verify its integration with the broader system.

✅ Verification successful

SMTP Transport implementation is properly integrated

The verification shows that the SMTP transport is well-integrated into the email system:

  • It correctly implements the MailTransport interface with both send and sendPlainText methods
  • The transport is properly exported and registered in the factory pattern via constructMailTransport
  • Configuration is properly typed with SmtpMailTransportOptions containing all necessary SMTP settings (host, port, secure, auth)
  • The transport type is correctly configured in the environment setup (EMAIL_TRANSPORT and ALERT_EMAIL_TRANSPORT enums)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for proper integration with the email client system

# Verify the transport is properly exported and used
rg -l "SmtpMailTransport" --type ts

# Check for configuration handling in the main application
rg "EMAIL_TRANSPORT.*smtp" --type ts

# Look for test coverage
rg -l "describe.*SmtpMailTransport" --type ts

Length of output: 423


Script:

#!/bin/bash
# Let's check the transport configuration and integration

# Check how transports are registered and used
cat internal-packages/emails/src/transports/index.ts

# Check the environment configuration structure
ast-grep --pattern 'interface $_Config {
  $$$
}'

# Look for SMTP configuration usage
rg "smtp.*config" --type ts -A 3

Length of output: 1555


Script:

#!/bin/bash
# Let's check the SMTP configuration structure and usage

# Check SMTP configuration type
ast-grep --pattern 'type SmtpMailTransportOptions = {
  $$$
}'

# Look for SMTP configuration setup
rg "type.*=.*smtp" --type ts -A 3

# Check for SMTP configuration validation
rg "smtp.*validate" --type ts -A 3

Length of output: 943

@erin-allison
Copy link
Contributor Author

I'll be honest... I have no idea what's causing the typecheck to fail. I didn't know to run the typecheck command to validate, but now that I have run it, I get the same error locally.

Error: webapp:typecheck: app/services/email.server.ts(3,38): error TS2307: Cannot find module 'emails/transports' or its corresponding type declarations.

My IDE doesn't say anything's wrong and the code works. I'm guessing it's something small that I missed. Can someone have a look and point me in the right direction?

Copy link
Member

@matt-aitken matt-aitken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TypeScript error when pnpm run typecheck --filter webapp is run. I've added a comment about why it's happening and an easy fix.

Having the default transport as undefined makes sense. It means people upgrading to this version will have to set some things to get alert emails to work again which is a bit annoying but I think on balance it's the right call because it's a better default for the future.

This is great, thanks!

Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 760887f and 4dcd770.

📒 Files selected for processing (2)
  • apps/webapp/app/services/email.server.ts (3 hunks)
  • internal-packages/emails/src/index.tsx (4 hunks)
🔇 Additional comments (5)
apps/webapp/app/services/email.server.ts (2)

Line range hint 66-124: LGTM: Email sending functions remain unchanged

The existing email sending functions correctly utilize the new transport system through the client instances.


2-2: LGTM: Client initialization with new transport options

The changes correctly integrate the new transport system while maintaining backward compatibility.

Let's verify the exports from the emails package:

Also applies to: 16-16, 27-27

✅ Verification successful

The previous command didn't yield results, possibly because:

  1. The export syntax might be different
  2. We need to locate the actual file first
  3. The pattern might need adjustment

Let's also check the actual type/interface definition:


MailTransportOptions type is correctly exported and matches the import

The verification confirms that MailTransportOptions is properly exported from the emails package as a union type of all supported mail transport options (AWS SES, Resend, SMTP, and Null transport) in internal-packages/emails/src/transports/index.ts. The import in apps/webapp/app/services/email.server.ts correctly references this type.

  • Location of type definition: internal-packages/emails/src/transports/index.ts
  • Type is a union of:
    • AwsSesMailTransportOptions
    • ResendMailTransportOptions
    • NullMailTransportOptions
    • SmtpMailTransportOptions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the exports from the emails package
ast-grep --pattern 'export { $_ as MailTransportOptions }'

Length of output: 60


Script:

#!/bin/bash
# Let's try a different approach to verify the exports
# First, let's find the emails package location
fd -t f "index.ts" -E "node_modules"

# Then, let's check for MailTransportOptions export using ripgrep
rg "export.*MailTransportOptions" -g "*.ts"

Length of output: 2453


Script:

#!/bin/bash
# Search for MailTransportOptions definition
ast-grep --pattern 'interface MailTransportOptions {
  $$$
}'

# Also check for type definition
ast-grep --pattern 'type MailTransportOptions = $_'

Length of output: 4228

internal-packages/emails/src/index.tsx (3)

Line range hint 2-18: LGTM: Import organization and type exports

The imports are well-organized and the necessary types are correctly exported.


43-56: LGTM: EmailClient transport abstraction

The EmailClient class correctly implements the transport abstraction, allowing for multiple email transport options while maintaining a clean interface.


67-81: LGTM: Email sending methods

The email sending methods properly utilize the transport abstraction while maintaining all necessary metadata (from, replyTo).

Signed-off-by: Erin Allison <eallison@andrettikarting.com>
Copy link

pkg-pr-new bot commented Dec 29, 2024

@trigger.dev/build

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/build@1580

trigger.dev

npm i https://pkg.pr.new/triggerdotdev/trigger.dev@1580

@trigger.dev/core

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/core@1580

@trigger.dev/react-hooks

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/react-hooks@1580

@trigger.dev/rsc

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/rsc@1580

@trigger.dev/sdk

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/sdk@1580

commit: 98946c2

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
apps/webapp/app/services/email.server.ts (2)

54-54: ⚠️ Potential issue

Fix incorrect environment variable reference

The SMTP secure flag is using the wrong environment variable for non-alert cases.

-          secure: alerts ? env.ALERT_SMTP_SECURE : env.ALERT_SMTP_SECURE,
+          secure: alerts ? env.ALERT_SMTP_SECURE : env.SMTP_SECURE,

34-64: 🛠️ Refactor suggestion

Add comprehensive validation and logging

The function should validate configurations and provide meaningful feedback when falling back to console transport.

The previous review suggested adding validation, and I agree. Here's a refined version that includes validation for all transport types and comprehensive logging:

 function buildTransportOptions(alerts?: boolean): MailTransportOptions {
   const transportType = alerts ? env.ALERT_EMAIL_TRANSPORT : env.EMAIL_TRANSPORT
   logger.debug(`Constructing email transport '${transportType}' for usage '${alerts?'alerts':'general'}'`)
 
+  // Helper to get the appropriate env var based on alert status
+  const getEnvVar = (alertVar: string, normalVar: string) => 
+    alerts ? process.env[alertVar] : process.env[normalVar];
+
   switch (transportType) {
     case "aws-ses":
-      return { type: "aws-ses" };
+      const awsRegion = getEnvVar('ALERT_AWS_REGION', 'AWS_REGION');
+      if (!awsRegion) {
+        logger.warn("AWS SES region not configured, falling back to console transport");
+        return { type: undefined };
+      }
+      return { 
+        type: "aws-ses",
+        config: { region: awsRegion }
+      };
     case "resend":
+      const resendApiKey = getEnvVar('ALERT_RESEND_API_KEY', 'RESEND_API_KEY');
+      if (!resendApiKey) {
+        logger.warn("Resend API key not configured, falling back to console transport");
+        return { type: undefined };
+      }
       return {
         type: "resend",
         config: {
-          apiKey: alerts ? env.ALERT_RESEND_API_KEY : env.RESEND_API_KEY,
+          apiKey: resendApiKey
         }
       }
     case "smtp":
+      const host = getEnvVar('ALERT_SMTP_HOST', 'SMTP_HOST');
+      const port = getEnvVar('ALERT_SMTP_PORT', 'SMTP_PORT');
+      if (!host || !port) {
+        logger.warn("SMTP host or port not configured, falling back to console transport");
+        return { type: undefined };
+      }
+      const user = getEnvVar('ALERT_SMTP_USER', 'SMTP_USER');
+      const pass = getEnvVar('ALERT_SMTP_PASSWORD', 'SMTP_PASSWORD');
+      if (!user || !pass) {
+        logger.warn("SMTP credentials not configured, falling back to console transport");
+        return { type: undefined };
+      }
       return {
         type: "smtp",
         config: {
-          host: alerts ? env.ALERT_SMTP_HOST : env.SMTP_HOST,
-          port: alerts ? env.ALERT_SMTP_PORT : env.SMTP_PORT,
+          host,
+          port: Number(port),
-          secure: alerts ? env.ALERT_SMTP_SECURE : env.ALERT_SMTP_SECURE,
+          secure: getEnvVar('ALERT_SMTP_SECURE', 'SMTP_SECURE') === 'true',
           auth: {
-            user: alerts ? env.ALERT_SMTP_USER : env.SMTP_USER,
-            pass: alerts ? env.ALERT_SMTP_PASSWORD : env.SMTP_PASSWORD
+            user,
+            pass
           }
         }
       };
     default:
+      logger.warn(`Unknown or missing email transport type '${transportType}', falling back to console transport`);
       return { type: undefined };
   }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4dcd770 and 98946c2.

📒 Files selected for processing (1)
  • apps/webapp/app/services/email.server.ts (3 hunks)
🔇 Additional comments (2)
apps/webapp/app/services/email.server.ts (2)

16-16: LGTM! Clean implementation of transport configuration

The changes effectively integrate the new transport system while maintaining the singleton pattern and providing sensible defaults.

Also applies to: 27-27


2-2: Verify the import path for MailTransportOptions

Based on previous typecheck errors mentioned in the PR comments, ensure that MailTransportOptions is properly exported from the main emails package.

✅ Verification successful

Import path for MailTransportOptions is correct

The MailTransportOptions type is properly exported from the main emails package. It's defined in internal-packages/emails/src/transports/index.ts and re-exported in internal-packages/emails/src/index.tsx. The import statement in apps/webapp/app/services/email.server.ts is valid and will work correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if MailTransportOptions is exported from the main package
ast-grep --pattern 'export { $_ as MailTransportOptions }' 
ast-grep --pattern 'export type MailTransportOptions'

Length of output: 116


Script:

#!/bin/bash
# Let's try a broader search to find any exports or type definitions related to MailTransportOptions
rg -l "MailTransportOptions"

# Search for any file that might be the main entry point of the emails package
fd -e ts -e js "index" packages/emails/

Length of output: 498


Script:

#!/bin/bash
# Let's check the content of the main entry point to verify the export
cat internal-packages/emails/src/index.tsx

# Also check the transports/index.ts as it might be re-exporting the type
cat internal-packages/emails/src/transports/index.ts

Length of output: 5509

Copy link
Member

@matt-aitken matt-aitken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant!

@matt-aitken matt-aitken merged commit 4d2412a into triggerdotdev:main Dec 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TRI-951] Remove resend dependency from the webapp and replace with nodemailer and smtp
2 participants