Skip to content

Conversation

@HeathHopkins
Copy link

The previous behavior only allowed email alerts in self-hosting if ALERT_RESEND_API_KEY was defined. This commit updates the environment check to also use ALERT_SMTP_HOST for alert email configuration. The ALERT_SMTP_HOST is already used in the SMPT client, so no other changes are necessary.

Closes #2618

✅ Checklist

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

Testing

None


Changelog

I modified the assignment of emailAlertsEnabled to also check ALERT_SMTP_HOST in addition to ALERT_RESEND_API_KEY when determining if alerts are enabled.


Screenshots

None

The previous behavior only allowed email alerts in self-hosting if ALERT_RESEND_API_KEY was defined.  This commit updates the environment check to also use ALERT_SMTP_HOST for alert email configuration.  The ALERT_SMTP_HOST is already used in the SMPT client, so no other changes are necessary.
@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2025

⚠️ No Changeset found

Latest commit: b63d003

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

Walkthrough

A single file within the alerts creation route handler was modified to adjust the condition for enabling email alerts. The loader function's emailAlertsEnabled flag now requires ALERT_FROM_EMAIL to be defined alongside either ALERT_RESEND_API_KEY or ALERT_SMTP_HOST, rather than requiring both ALERT_RESEND_API_KEY and ALERT_FROM_EMAIL. This expands the configuration options supported for email alert delivery by incorporating SMTP as an alternative to the Resend API.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Rationale: Single file modification affecting one conditional expression in a loader function. The change is straightforward—adding an OR clause to broaden email alert enablement conditions. Requires basic understanding of the email alert configuration options but involves minimal logic density and no related structural changes. No public API modifications.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Allow alerts to be created if ALERT_SMTP_HOST is defined" directly and specifically describes the main change in the pull request. The loader's emailAlertsEnabled condition was broadened to include checking for ALERT_SMTP_HOST in addition to ALERT_RESEND_API_KEY, which is precisely what the title conveys. The title is concise, clear, and specific enough for a teammate reviewing history to understand the primary change without being vague or generic.
Linked Issues Check ✅ Passed Issue #2618 requested modification of the emailAlertsEnabled variable to check for process.env.ALERT_SMTP_HOST to enable email alerts on self-hosted deployments using SMTP. The code changes directly fulfill this requirement by broadening the emailAlertsEnabled condition to check for either ALERT_RESEND_API_KEY or ALERT_SMTP_HOST in addition to ALERT_FROM_EMAIL, enabling SMTP-configured instances to create email alerts without requiring Resend configuration.
Out of Scope Changes Check ✅ Passed The pull request contains changes to only a single file (the alerts.new route), with modifications focused exclusively on the emailAlertsEnabled condition to add the ALERT_SMTP_HOST check. This aligns directly with the scope defined in issue #2618, which specifically requested updating the emailAlertsEnabled variable. The author notes that ALERT_SMTP_HOST is already used in the SMTP client, confirming that no other changes were necessary, indicating all changes are appropriately scoped.
Description Check ✅ Passed The pull request description follows the provided template structure and includes all major required sections: issue reference (Closes #2618), checklist, testing section, changelog, and screenshots. The description provides clear context about the change, explaining that the previous behavior only checked for ALERT_RESEND_API_KEY and that the update now also considers ALERT_SMTP_HOST. While one checklist item (testing) remains unchecked, the description is substantially complete and communicates the necessary information for understanding the change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3157b65 and b63d003.

📒 Files selected for processing (1)
  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

We use zod a lot in packages/core and in the webapp

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx
apps/webapp/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)

When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json

Files:

  • apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx
🔇 Additional comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx (1)

160-161: Change is correct and properly gated.

The condition now correctly enables email alerts when ALERT_FROM_EMAIL is configured alongside either ALERT_RESEND_API_KEY or ALERT_SMTP_HOST. Verification confirms:

  • The email service (apps/webapp/app/services/email.server.ts) already implements both Resend and SMTP transports with proper alert-specific environment variables
  • This is the only place in the codebase that gates email alert availability
  • No other conditional checks need updating

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

feat: Allow email alerts on self-hosted deploys when using SMTP

1 participant