Conversation
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
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="packages/features/ee/workflows/lib/service/EmailWorkflowService.ts">
<violation number="1" location="packages/features/ee/workflows/lib/service/EmailWorkflowService.ts:418">
P1: Potential regression when `reminderBody` is null but `template` is REMINDER/RATING. The old code would generate the default template from the `template` parameter, but the new code only generates templates when `matchedTemplate` is set (which requires `emailBody` to be truthy). Consider adding a fallback to check the `template` parameter when `emailBody` is empty:
```javascript
} else if (emailBody) {
// custom processing
} else if (template === WorkflowTemplates.REMINDER) {
// Generate REMINDER with correct locale (fallback for empty body)
} else if (template === WorkflowTemplates.RATING) {
// Generate RATING with correct locale (fallback for empty body)
}
```</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
CarinaWolli
previously requested changes
Jan 8, 2026
Member
CarinaWolli
left a comment
There was a problem hiding this comment.
We are only checking for changes in the email body and not the subject. So if only the subject was changed for the reminder template, then we are doing the translation using the default reminder subject instead of sending the custom email subject
Co-Authored-By: morgan@cal.com <morgan@cal.com>
Contributor
E2E results are ready! |
hariombalhara
previously requested changes
Jan 13, 2026
Member
hariombalhara
left a comment
There was a problem hiding this comment.
There is a scope of code quality improvement and I think tests are also required
packages/features/ee/workflows/lib/service/EmailWorkflowService.ts
Outdated
Show resolved
Hide resolved
- Create detectMatchedTemplate function in separate file - Add comprehensive unit tests (20 test cases) - Refactor EmailWorkflowService to use the new function Addresses review comment from hariombalhara requesting abstraction and tests for the template matching logic. Co-Authored-By: udit@cal.com <udit222001@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR fixes the issue when workflow reminder body isn't translated to attendee's locale.
Updates since last revision
Latest (hariombalhara's feedback): Extracted the template matching logic into a separate testable function with comprehensive unit tests.
Changes:
detectMatchedTemplatefunction inpackages/features/ee/workflows/lib/detectMatchedTemplate.tsEmailWorkflowServiceto use the new functionPrevious (CarinaWolli's feedback): Now checks both email body AND subject against default templates before regenerating with recipient's locale. Previously only the body was checked, which meant a customized subject would be overwritten with the default when the body matched the template.
Changes:
getTemplateSubjectForActionhelper function to get default subject for templatescustomTemplate()path to preserve user editsMandatory Tasks (DO NOT REMOVE)
How should this be tested?
Human Review Checklist
detectMatchedTemplatefunction behavior matches the original inline logicdetectMatchedTemplate.test.tsfor completenessLink to Devin run: https://app.devin.ai/sessions/fd81f5a877f447aaa8112dd55148953d
Requested by: udit@cal.com (@Udit-takkar)