Skip to content

Better error handling for failed emails #1540

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.

Sign up for GitHub

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 1 commit into from
Dec 10, 2024
Merged

Better error handling for failed emails #1540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion internal-packages/emails/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,21 @@ export class EmailClient {

async #sendEmail({ to, subject, react }: { to: string; subject: string; react: ReactElement }) {
if (this.#client) {
await this.#client.emails.send({
const result = await this.#client.emails.send({
from: this.#from,
to,
reply_to: this.#replyTo,
subject,
react,
});

if (result.error) {
console.error(
`Failed to send email to ${to}, ${subject}. Error ${result.error.name}: ${result.error.message}`
);
throw new EmailError(result.error);
}

Comment on lines +130 to +144
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the gathered information, I can now provide a final response:

EmailError handling needs to be added to webapp email service

The new EmailError is thrown in the email package but the webapp's email service (apps/webapp/app/services/email.server.ts) doesn't handle it specifically:

  • sendEmail and sendAlertEmail functions directly return the client calls without error handling
  • Only sendMagicLinkEmail has a try-catch block, but it catches and rethrows generic errors
  • sendPlainTextEmail has no error handling

Add specific EmailError handling to these functions to maintain consistent error handling across the codebase.

🔗 Analysis chain

Verify error handling in calling code

Let's ensure that all callers of #sendEmail are prepared to handle the new EmailError type.

Also applies to: 158-164

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct calls to send() method and verify error handling
rg -l "emailClient\.send\(" | while read -r file; do
  echo "=== Checking $file ==="
  # Show the context around send() calls to verify error handling
  rg -B 2 -A 4 "emailClient\.send\(" "$file"
done

# Search for any existing try-catch blocks that might need updating
rg "catch.*EmailError"

Length of output: 69

Script:

#!/bin/bash
# Let's try a different approach to find email sending patterns and error handling

# First, let's find all files that might contain email-related code
echo "=== Files containing email-related code ==="
rg -l -i "email" 

# Look for method calls on any email client/service
echo -e "\n=== Email sending patterns ==="
rg -A 3 -B 3 "\.send\(" 

# Look for error handling patterns around email
echo -e "\n=== Error handling patterns ==="
rg -A 3 -B 3 "try.*{.*send"

# Check for any existing EmailError references
echo -e "\n=== EmailError usage ==="
rg -l "EmailError"

Length of output: 49183

Script:

#!/bin/bash
# Let's find all places where the email client is used and check error handling
echo "=== Files importing or using the email client ==="
rg -l "from.*['\"]@trigger\.dev/emails['\"]|from.*['\"]internal-packages/emails['\"]"

# Let's check the implementation and usage of EmailError
echo -e "\n=== EmailError implementation and usage ==="
rg -A 5 -B 5 "class.*EmailError|new EmailError"

# Let's look for try-catch blocks around email sending
echo -e "\n=== Error handling around email sending ==="
rg -A 5 -B 5 "try.*{.*#sendEmail|try.*{.*emailClient\.send"

Length of output: 1988

Script:

#!/bin/bash
# Let's try a different approach to find error handling patterns
echo "=== Error handling patterns around email sending ==="
rg -A 5 -B 5 "try\s+{[\s\S]*?sendEmail|try\s+{[\s\S]*?emails\.send"

# Let's find all files that use the email client
echo -e "\n=== Files using email client ==="
rg -l "new EmailClient|EmailClient\(" 

# Let's check for any imports of the email module
echo -e "\n=== Email module imports ==="
rg -l "from ['\"](\.\.\/)*emails['\"]|require\(['\"].*emails['\"]"

Length of output: 773

Script:

#!/bin/bash
# Let's examine the email client usage in the webapp
echo "=== Email client implementation in webapp ==="
cat "apps/webapp/app/services/email.server.ts"

# Let's look for try-catch blocks with simpler pattern
echo -e "\n=== Error handling patterns ==="
rg -A 5 -B 5 "try.*{.*send"

# Let's check for any email-related error handling
echo -e "\n=== Email error handling ==="
rg -A 5 -B 5 "catch.*{.*email"

Length of output: 3139

Copy link
Member

Choose a reason for hiding this comment

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

@matt-aitken is this true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but those functions are only called by Graphile jobs, so don't we want to throw so the jobs get retried.

sendEmail:

trigger.dev/apps/webapp/app/services/worker.server.ts

Line 444 in 9065e64

await sendEmail(payload);

sendAlertEmail is called in DeliverAlertService which is called from here:

trigger.dev/apps/webapp/app/services/worker.server.ts

Line 626 in 9065e64

const service = new DeliverAlertService();

return;
}

Expand All @@ -147,3 +154,11 @@ ${render(react, {
`);
}
}

//EmailError type where you can set the name and message
export class EmailError extends Error {
constructor({ name, message }: { name: string; message: string }) {
super(message);
this.name = name;
}
}
Loading