Better error handling for failed emails #1540
Conversation
|
Caution Review failedThe pull request is closed. 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
internal-packages/emails/src/index.tsxOops! 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. WalkthroughThe changes in this pull request focus on enhancing error handling within the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
internal-packages/emails/src/index.tsx (2)
130-144: LGTM! Consider enhancing error loggingThe new error handling implementation properly captures and propagates email provider errors, which addresses the PR objective.
Consider enhancing the error logging to include more context:
- console.error( - `Failed to send email to ${to}, ${subject}. Error ${result.error.name}: ${result.error.message}` - ); + console.error( + `Failed to send email to ${to}, subject: "${subject}". ` + + `Provider error [${result.error.name}]: ${result.error.message}`, + { error: result.error } + );
158-164: Add documentation and type safety to EmailError classThe
EmailErrorclass implementation is correct, but could benefit from better documentation and type safety.Consider these improvements:
+/** Known error names from the email provider */ +type EmailErrorName = + | "rate_limit_exceeded" + | "invalid_recipient" + | "delivery_error" + | string; + +/** + * Custom error class for email-related errors + * @extends Error + */ export class EmailError extends Error { + name: EmailErrorName; + constructor({ name, message }: { name: string; message: string }) { super(message); this.name = name; } }
| 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 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:
sendEmailandsendAlertEmailfunctions directly return the client calls without error handling- Only
sendMagicLinkEmailhas a try-catch block, but it catches and rethrows generic errors sendPlainTextEmailhas 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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Yes but those functions are only called by Graphile jobs, so don't we want to throw so the jobs get retried.
sendEmail:
sendAlertEmail is called in DeliverAlertService which is called from here:
ericallam
force-pushed
the
email-error-handling
branch
from
c5b69eb to
7d6db03
Compare
|
If we got an email back from our email provider we weren't propagating that error back up, causing a retry. Their SDK changed to not throw errors at some point.
Summary by CodeRabbit
New Features
EmailErrorclass to enhance error clarity.Bug Fixes