fix: Show error message when SMTP is not configured#38546
fix: Show error message when SMTP is not configured#38546SnehaChauhan1174 wants to merge 5 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
WalkthroughAdds a runtime SMTP configuration check inside the mailer send flow: when MAIL_URL is not set, the code inspects SMTP host/port/username/password from settings and throws a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![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
🤖 Fix all issues with AI agents
In `@apps/meteor/app/mailer/server/api.ts`:
- Around line 161-167: The block that retrieves SMTP_Host uses spaces for
indentation instead of tabs, causing a style mismatch; locate the smtpHost
assignment and the subsequent if/throw that uses
settings.get<string>('SMTP_Host') and Meteor.Error, and re-indent those lines
with tabs to match the file's existing tab-based formatting so
linting/formatters no longer flag the diff.
🧹 Nitpick comments (1)
apps/meteor/app/mailer/server/api.ts (1)
163-166: Consider using an i18n key instead of a hardcoded English string for the user-facing error reason.The second argument to
Meteor.Erroris surfaced to end-users. Other parts of this file usei18n.t(key, { lng })for translated strings. A hardcoded English sentence won't be translated for non-English deployments.That said, existing
Meteor.Errorcalls in this file (e.g.,'invalid email'on Line 159) also use plain strings, so this is consistent with the current pattern — worth addressing holistically rather than just here.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/mailer/server/api.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/mailer/server/api.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/app/mailer/server/api.ts (1)
161-167: No action needed. The setting keySMTP_Hostis correctly registered in the settings registry atapps/meteor/server/settings/email.tsand is used consistently throughout the codebase with matching identifiers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const smtpHost = settings.get<string>('SMTP_Host'); | ||
| if (!smtpHost) { | ||
| throw new Meteor.Error( | ||
| 'smtp-not-configured', | ||
| 'SMTP is not configured. Please configure it in Administration -> Settings -> Email -> SMTP before sending emails.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Indentation mismatch: uses 4 spaces instead of tabs.
Lines 161–167 are indented with spaces while the rest of the file consistently uses tabs. This will cause linting/formatting failures or at minimum a noisy diff.
🔧 Proposed fix — re-indent with tabs to match the file
- const smtpHost = settings.get<string>('SMTP_Host');
- if (!smtpHost) {
- throw new Meteor.Error(
- 'smtp-not-configured',
- 'SMTP is not configured. Please configure it in Administration -> Settings -> Email -> SMTP before sending emails.',
- );
- }
+ const smtpHost = settings.get<string>('SMTP_Host');
+ if (!smtpHost) {
+ throw new Meteor.Error(
+ 'smtp-not-configured',
+ 'SMTP is not configured. Please configure it in Administration -> Settings -> Email -> SMTP before sending emails.',
+ );
+ }As per coding guidelines, "Write concise, technical TypeScript/JavaScript" — the formatting should be consistent with the rest of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const smtpHost = settings.get<string>('SMTP_Host'); | |
| if (!smtpHost) { | |
| throw new Meteor.Error( | |
| 'smtp-not-configured', | |
| 'SMTP is not configured. Please configure it in Administration -> Settings -> Email -> SMTP before sending emails.', | |
| ); | |
| } | |
| const smtpHost = settings.get<string>('SMTP_Host'); | |
| if (!smtpHost) { | |
| throw new Meteor.Error( | |
| 'smtp-not-configured', | |
| 'SMTP is not configured. Please configure it in Administration -> Settings -> Email -> SMTP before sending emails.', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/app/mailer/server/api.ts` around lines 161 - 167, The block that
retrieves SMTP_Host uses spaces for indentation instead of tabs, causing a style
mismatch; locate the smtpHost assignment and the subsequent if/throw that uses
settings.get<string>('SMTP_Host') and Meteor.Error, and re-indent those lines
with tabs to match the file's existing tab-based formatting so
linting/formatters no longer flag the diff.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/mailer/server/api.ts">
<violation number="1" location="apps/meteor/app/mailer/server/api.ts:161">
P2: New SMTP_Host-only check blocks valid email configurations that rely on MAIL_URL; use the existing isSMTPConfigured logic instead of hard-requiring SMTP_Host.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Hi @SnehaChauhan1174 ! Let's fix the administrative blocks first:
How to fix: You can change the title by clicking the "Edit" button at the top of this page (to the right of the current title or in checks). Please follow the Conventional Commits format.
How to fix: Try clicking the 'recheck' link in the CLAassistant comment. You can also go to the checks list at the bottom of the page, click on "license/cla", and ensure everything is authorized. Once these are green, we can look into the linting and logic issues! |
SnehaChauhan1174
changed the title
SnehaChauhan1174
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/mailer/server/api.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/mailer/server/api.ts
🧠 Learnings (2)
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/mailer/server/api.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/mailer/server/api.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/mailer/server/api.ts`:
- Around line 163-168: isSMTPConfigured currently returns true if
settings.get('SMTP_Host') or process.env.MAIL_URL is present, but that allows
incomplete credentials; update the isSMTPConfigured function so it returns true
only when there is either a valid MAIL_URL OR all required SMTP settings are
present and non-empty (at minimum check SMTP_Host, SMTP_Port, SMTP_Username,
SMTP_Password and any TLS flag like SMTP_UseTLS if your code expects it), and
keep the guard around the throw in the same block that calls isSMTPConfigured
(the code that throws 'smtp-not-configured' in api.ts). If you parse MAIL_URL,
validate it contains auth and host:port; otherwise ensure
settings.get('SMTP_Host'), settings.get('SMTP_Port'),
settings.get('SMTP_Username'), and settings.get('SMTP_Password') are truthy
before allowing Email.sendAsync to run.
---
Duplicate comments:
In `@apps/meteor/app/mailer/server/api.ts`:
- Around line 162-168: The new SMTP check block uses spaces (and mixed
space+tab) instead of the file's tab indentation; update the if-block that calls
isSMTPConfigured() and the nested throw new Meteor.Error(...) so each line uses
tabs (no spaces) and matches the surrounding tab-based indentation style,
removing any mixed whitespace to satisfy lint/format rules.
4 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
fixes #38543
Summary by CodeRabbit
Bug Fixes
Chores