chore: Read mail env variables only when needed #37660
Conversation
WalkthroughThe changes in this pull request involve modifying the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
sharat87
requested review from
mohanarpit,
nidhi-nair,
trishaanand and
pratapaprasanna
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/packages/rts/src/ctl/mailer.ts (4)
7-13: Consider adding type annotations for environment variablesWhile moving the environment variables into the function scope is good for lazy loading, consider adding proper TypeScript types for better type safety.
- const mailEnabled = process.env.APPSMITH_MAIL_ENABLED; - const mailPort = process.env.APPSMITH_MAIL_PORT; + const mailEnabled = process.env.APPSMITH_MAIL_ENABLED as string; + const mailPort = parseInt(process.env.APPSMITH_MAIL_PORT ?? '', 10);
Line range hint
17-35: Simplify validation logic and remove redundant checkThe validation contains a redundant check for
mailEnabledand could be simplified.if ( !mailEnabled || !mailFrom || !mailHost || !mailPort || !mailUser || !mailPass ) { throw new Error( "Failed to send error mail. Email provider is not configured, please refer to https://docs.appsmith.com/setup/instance-configuration/email to configure it.", ); } else if (!mailTo) { throw new Error( "Failed to send error mail. Admin email(s) not configured, please refer to https://docs.appsmith.com/setup/instance-configuration/disable-user-signup#administrator-emails to configure it.", ); - } else if (!mailEnabled) { - throw new Error( - "Mail not sent! APPSMITH_MAIL_ENABLED env val is disabled, please refer to https://docs.appsmith.com/setup/instance-configuration/email to enable it.", - ); } else {
Line range hint
55-71: Improve string handling and URL constructionThe current implementation uses string concatenation and lacks URL validation.
- let text = - "Appsmith backup did not complete successfully.\n\n " + - "Backup timestamp: " + - backupTimestamp + - "\n\n" + - "Last Successful Backup timestamp: " + - lastBackupTimestamp; + let text = `Appsmith backup did not complete successfully. + +Backup timestamp: ${backupTimestamp} + +Last Successful Backup timestamp: ${lastBackupTimestamp}`; if (domainName) { + const protocol = domainName.startsWith('http') ? '' : 'http://'; text = - text + - "Link to Appsmith admin settings: " + - "http://" + - domainName + - "/settings/general"; + `${text} +Link to Appsmith admin settings: ${protocol}${domainName}/settings/general`; }
Line range hint
82-97: Improve email transport configurationThe email transport configuration could be improved with proper types and timeout.
+ const port = typeof mailPort === 'string' ? parseInt(mailPort, 10) : mailPort; const transporter = nodemailer.createTransport({ host: mailHost, - port: mailPort, + port, auth: { user: mailUser, pass: mailPass, - }, + }, + pool: true, + maxConnections: 1, + rateDelta: 20000, + rateLimit: 5, } as any);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/packages/rts/src/ctl/mailer.ts(1 hunks)
🔇 Additional comments (1)
app/client/packages/rts/src/ctl/mailer.ts (1)
6-14: Verify the timing of environment variable loading
The changes correctly move environment variable loading to runtime, addressing the original issue. Let's verify there are no other instances of mail-related environment variables being loaded at import time.
✅ Verification successful
Let me verify if these environment variables are accessed at import time by checking if they are used outside of functions.
Environment variables are correctly accessed at runtime
All mail-related environment variables are accessed within the sendBackupErrorToAdmins function and not at import time. The changes successfully address the original concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining mail-related environment variable access at import time
rg "process\.env\.APPSMITH_MAIL" --type ts
Length of output: 616
Script:
#!/bin/bash
# Show the full context of mailer.ts to verify if env vars are accessed at import time
cat app/client/packages/rts/src/ctl/mailer.ts
Length of output: 3056
Loading env variables at import time doesn't work now since we're importing first, and them loading env variables, after having moved to TypeScript. This PR fixes that in the mailer module. ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!CAUTION] > If you modify the content in this section, you are likely to disrupt the CI result for your PR. <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved error handling for email configuration by ensuring environment variables are validated before sending emails. - **Refactor** - Adjusted the scope of several environment variables to enhance code organization and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Loading env variables at import time doesn't work now since we're importing first, and them loading env variables, after having moved to TypeScript.
This PR fixes that in the mailer module.
Automation
/test sanity
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor