Fix email template loading and error handling for infrastructure alerts #3059
Conversation
…ervice / Infrastructure alerts emails are empty bluewave-labs#3054
Retr0-XD
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Template initialization and caching server/src/service/v1/infrastructure/emailService.js |
Changed init method from async to synchronous with preloaded templates; added loadTemplate() method for loading and compiling templates; introduced templateLookup object for caching compiled templates; enhanced buildEmail() with lazy-loading fallback and template validation; improved error handling with descriptive logging and re-throws |
Sequence Diagram
sequenceDiagram
participant Client
participant EmailService
participant TemplateSystem
Note over Client,TemplateSystem: Initialization Phase
Client->>EmailService: new EmailService()
EmailService->>EmailService: init() [synchronous]
EmailService->>TemplateSystem: loadTemplate() for each
TemplateSystem-->>EmailService: compiled functions cached
Note over EmailService: templateLookup populated
Note over Client,TemplateSystem: Email Building Phase
Client->>EmailService: buildEmail(templateName, ...)
EmailService->>EmailService: Check templateLookup
alt Template Found
EmailService->>TemplateSystem: Use cached template
else Template Not Found
EmailService->>TemplateSystem: loadTemplate() with fallback logic
TemplateSystem-->>EmailService: compiled function or null
alt Load Success
EmailService->>EmailService: Cache in templateLookup
else Load Fails
EmailService->>EmailService: Log & throw error
EmailService-->>Client: Error
end
end
EmailService->>TemplateSystem: Generate MJML → HTML
EmailService-->>Client: HTML or empty string
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Areas requiring attention:
- Logic in
buildEmail()for template fallback resolution and lazy-loading behavior - Error handling paths and their impact on caller expectations
- Verify all templates that should be preloaded are covered in
init() - Confirm synchronous initialization doesn't block the application startup
- Logic in
Poem
🐰 Templates now load without a wait,
No empty emails, their proper fate!
Preloaded and cached, they're ready to go,
Infrastructure alerts will always show! 📧✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description provides context and includes the issue reference, but it lacks details on verification steps and doesn't follow the provided template with required checklist items. | Add a completed checklist from the template showing local deployment, self-review, and testing verification to confirm the fix resolves the empty email issue. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and specifically describes the main change: fixing email template loading and error handling for infrastructure alerts, which directly addresses the linked issue. |
| Linked Issues check | ✅ Passed | The code changes directly address all key requirements from issue #3054: fixing template loading errors, preventing empty emails, and improving error handling through synchronous initialization and lazy-loading with caching. |
| Out of Scope Changes check | ✅ Passed | All changes are focused on the EmailService template loading mechanism and error handling, directly scoped to fixing the infrastructure alert email issue with no unrelated modifications. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
![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
🧹 Nitpick comments (1)
server/src/service/v1/infrastructure/emailService.js (1)
51-67: Consider caching template load failures to avoid repeated disk I/O.When
loadTemplatereturnsnull(line 65), preloaded templates that fail will benullintemplateLookup. Later, whenbuildEmailencounters anulltemplate, it will attempt to lazy-load it again (line 100). If the template file is genuinely missing or has a persistent compilation error, this results in repeated filesystem reads and failed compilations on every call.Consider one of these approaches:
Option 1: Cache failures explicitly
this.loadTemplate = (templateName) => { try { const templatePath = this.path.join(__dirname, `../../../templates/${templateName}.mjml`); const templateContent = this.fs.readFileSync(templatePath, "utf8"); - // compile returns a function that accepts a context and returns the rendered mjml string return this.compile(templateContent); } catch (error) { this.logger.error({ message: error.message, service: SERVICE_NAME, method: "loadTemplate", stack: error.stack, }); - // Return null when template can't be loaded so callers can handle it explicitly - return null; + // Return false to distinguish cached failures from missing entries + return false; } };Then in
buildEmail(line 96), check forfalse:-if (typeof tplFn !== "function") { +if (tplFn === false) { + // Template previously failed to load, don't retry + throw new Error(`Template not found or failed to compile: ${template}`); +} +if (typeof tplFn !== "function") { const baseName = String(template).replace(/Template$/i, ""); tplFn = this.loadTemplate(baseName); - if (typeof tplFn === "function") { + if (tplFn === false) { + this.templateLookup[template] = false; // cache the failure + throw new Error(`Template not found or failed to compile: ${template}`); + } else if (typeof tplFn === "function") { this.templateLookup[template] = tplFn; } else { - this.logger.error({ - message: `Template not found or failed to compile: ${template}`, - service: SERVICE_NAME, - method: "buildEmail", - }); throw new Error(`Template not found or failed to compile: ${template}`); } }Option 2: Fail-fast during initialization
If any of the preloaded templates are critical, consider throwing an error during
init()when a required template fails to load.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/service/v1/infrastructure/emailService.js(4 hunks)
🔇 Additional comments (3)
server/src/service/v1/infrastructure/emailService.js (3)
40-44: LGTM: Synchronous initialization is appropriate.The change from async to sync initialization is correct since
loadTemplateuses synchronous file operations (readFileSync). The detailed comments clearly explain the rationale for preventing race conditions.
91-128: Excellent error handling! Addresses the PR objective of preventing empty emails.The enhanced error handling in
buildEmailcorrectly throws errors when templates fail to load or compile (line 126), preventing the system from silently sending empty emails. This directly addresses the issue described in #3054.The lazy-loading mechanism (lines 97-112) with caching is well-implemented and provides good fallback support for templates not preloaded during initialization.
Minor observation about line 116:
The fallback to empty string
|| ""might hide edge cases wheremjml2htmlreturns an unexpected value (e.g.,{html: null}). However, since any error during template execution or MJML conversion is caught and re-thrown (line 126), this fallback should only apply to successful conversions with falsy HTML, which is likely acceptable.Template naming convention (line 99):
The logic assumes template keys in
templateLookupend with "Template" suffix (e.g.,hardwareIncidentTemplate) while the actual.mjmlfiles use the base name (e.g.,hardwareIncident.mjml). This is consistent with the preloaded templates (lines 74-83) and should work correctly. Consider documenting this convention in the JSDoc forbuildEmailto help future maintainers.
74-83: All template files verified as present.Verification confirms that all 8 template files referenced in the
templateLookupinitialization exist in theserver/src/templates/directory. No issues found.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR enhances email template reliability but introduces a breaking change in error handling that could crash existing callers, along with an async/await mismatch that may return Promises instead of HTML strings.
📄 Documentation Diagram
This diagram documents the refactored email template loading and rendering flow with lazy-loading capabilities.
sequenceDiagram
participant C as Caller
participant E as EmailService
participant T as Template Loader
participant M as MJML Compiler
C->>E: buildEmail(template, context)
E->>E: Check templateLookup[template]
alt template is function
E->>E: tplFn(context)
else template not found
note over E: PR #35;3059 added lazy-loading logic
E->>T: loadTemplate(baseName)
T->>T: readFileSync and compile
alt template loaded
E->>E: Cache template in lookup
else template error
E->>E: Log error and return null
end
end
E->>M: mjml2html(mjml)
M-->>E: result
E-->>C: html or error
🌟 Strengths
- Addresses the core issue of empty emails by improving error handling and logging.
- Implements lazy-loading and caching for better performance and consistency.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/src/.../emailService.js | Architecture | Breaking change crashes callers without try-catch | method:buildEmail, path:settingsController |
| P1 | server/src/.../emailService.js | Bug | Async/await mismatch returns Promise instead of HTML | |
| P2 | server/src/.../emailService.js | Performance | Synchronous file reads may block event loop | |
| P2 | server/src/.../emailService.js | Maintainability | Implicit template naming could cause confusion | |
| P2 | server/src/.../emailService.js | Documentation | Error message lacks template path for debugging |
🔍 Notable Themes
- Error Handling Changes: The shift from silent failures to throwing errors requires all callers to handle exceptions, potentially breaking existing integrations.
- Asynchronous Consistency: Inconsistent use of async/await could lead to type errors and unexpected behavior in the email rendering pipeline.
📈 Risk Diagram
This diagram illustrates the breaking change in error handling and async/await mismatch risks introduced by the PR.
sequenceDiagram
participant C as Caller
participant E as EmailService
C->>E: buildEmail(template, context)
E->>E: Check if template is function
alt template not function
E->>E: Lazy load template
alt load fails
note over E: R1(P1): Throws error, breaking callers without try-catch
E-->>C: Error thrown
end
end
E->>E: Render MJML without await
note over E: R2(P1): May return Promise if mjml2html is async
E-->>C: Potential Promise instead of HTML
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| const mjml = this.templateLookup[template](context); | ||
| const html = await this.mjml2html(mjml); | ||
| return html.html; | ||
| // Ensure the template exists and is a function. Templates are stored in the lookup |
There was a problem hiding this comment.
P1 | Confidence: High
The change from silently returning undefined to throwing errors in buildEmail creates a breaking change for all callers. The related context shows settingsController.js calls buildEmail without try-catch, meaning template errors will now bubble up and crash the email test endpoint. This impacts method:buildEmail because existing callers expect graceful failure but will now receive unhandled exceptions.
Code Suggestion:
// In settingsController.js, wrap the buildEmail call:
try {
const html = await this.emailService.buildEmail("testEmailTemplate", context);
} catch (error) {
// Handle template error appropriately
throw this.errorService.createServerError("Failed to generate email template.");
}Evidence: path:server/src/controllers/v1/settingsController.js, method:buildEmail
| const mjml = tplFn(context); | ||
| const result = this.mjml2html(mjml); | ||
| return (result && result.html) || ""; |
There was a problem hiding this comment.
P1 | Confidence: High
The mjml2html function is called without await, but the method is marked async and the previous implementation used await. If mjml2html returns a Promise (asynchronous MJML compilation), this will return a Promise instead of the HTML string, breaking the email rendering pipeline and causing downstream type errors.
| const mjml = tplFn(context); | |
| const result = this.mjml2html(mjml); | |
| return (result && result.html) || ""; | |
| const mjml = tplFn(context); | |
| const result = await this.mjml2html(mjml); | |
| return (result && result.html) || ""; |
| // NOTE: This is intentionally synchronous so that templates are available | ||
| // immediately after the constructor returns (prevents race conditions | ||
| // where callers invoke buildEmail before templateLookup is populated). | ||
| init = () => { |
There was a problem hiding this comment.
P2 | Confidence: Medium
Synchronous file reads (readFileSync) during initialization could block the event loop, especially with many templates. While the change aims to prevent race conditions, it may impact application startup time and scalability.
Code Suggestion:
this.initialized = false;
init = async () => {
// ... async file operations
this.initialized = true;
};
// In buildEmail, check initialization status| // Attempt to lazy-load the template. Templates are named by dropping a trailing | ||
| // "Template" suffix (eg. hardwareIncidentTemplate -> hardwareIncident.mjml). | ||
| const baseName = String(template).replace(/Template$/i, ""); | ||
| tplFn = this.loadTemplate(baseName); |
There was a problem hiding this comment.
P2 | Confidence: Medium
The template naming convention (removing "Template" suffix) creates implicit coupling between template lookup keys and filesystem names. This could lead to confusion if new templates don't follow this pattern. The logic is also case-insensitive (/Template$/i) which might mask naming inconsistencies.
Code Suggestion:
const templateMappings = {
hardwareIncidentTemplate: 'hardwareIncident',
// ... other mappings
};
const baseName = templateMappings[template] || template.replace(/Template$/i, "");| } else { | ||
| this.logger.error({ | ||
| message: `Template not found or failed to compile: ${template}`, | ||
| service: SERVICE_NAME, | ||
| method: "buildEmail", | ||
| }); | ||
| throw new Error(`Template not found or failed to compile: ${template}`); | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Low
The error message doesn't include the resolved template path, making debugging more difficult when templates are missing. Developers would need to manually trace the template resolution logic.
| } else { | |
| this.logger.error({ | |
| message: `Template not found or failed to compile: ${template}`, | |
| service: SERVICE_NAME, | |
| method: "buildEmail", | |
| }); | |
| throw new Error(`Template not found or failed to compile: ${template}`); | |
| } | |
| throw new Error(`Template not found or failed to compile: ${template} (resolved to: ${baseName}.mjml)`); |
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
Hi @Retr0-XD
This looks like a good addition, however the behavior of buildEmail has chagned to now throw, so please go through the application and make sure that all callers of buildEmail are properly handling the new behavior.
Thanks!
2 participants
Enhancements to the EmailService improve the loading of email templates and error handling. The changes ensure that when a template fails to load or compile, the error is logged and re-thrown, preventing the sending of empty emails. This addresses the issue of empty infrastructure alert emails.
Fixes #3054
Summary by CodeRabbit