#3095 [FEAT] Add Twilio SMS Notification Support#3096
#3095 [FEAT] Add Twilio SMS Notification Support#3096khoazero123 wants to merge 5 commits intobluewave-labs:developfrom
Conversation
khoazero123
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Localization & Notification Configuration client/src/locales/en.json, client/src/Pages/v1/Notifications/utils.js |
Added Twilio translation keys and UI strings; added twilio entry to TITLE_MAP, DESCRIPTION_MAP, LABEL_MAP, PLACEHOLDER_MAP. |
Client Notification Types & Hooks client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx, client/src/Components/v1/NotificationIntegrationModal/Hooks/useNotification.js |
Introduced twilio notification type and UI field (phoneNumber); added test-send branch validating phone number and setting errorMessage/isValid before requests. |
Client Settings & Integration client/src/Pages/v1/Settings/SettingsTwilio.jsx, client/src/Pages/v1/Settings/index.jsx, client/src/Validation/validation.js |
New SettingsTwilio component (Account SID, Auth Token, Phone Number, admin-only) integrated into Settings page; added twilio fields to settingsValidation and notificationValidation (require phone number for Twilio). |
Server Dependency & Models server/package.json, server/src/db/v1/models/AppSettings.js, server/src/db/v1/models/Notification.js |
Added twilio npm dependency; added AppSettings fields twilioAccountSid, twilioAuthToken, twilioPhoneNumber; added "twilio" to Notification type enum. |
Server Services & Wiring server/src/config/services.js, server/src/service/v1/infrastructure/twilioService.js, server/src/service/v1/infrastructure/notificationService.js |
New TwilioService class (init from DB settings, sendSms -> boolean); instantiated and injected into NotificationService; NotificationService handles type === "twilio" by calling twilioService.sendSms(). |
Server Validation server/src/validation/joi.js |
Extended updateAppSettingsBodyValidation with Twilio fields; expanded createNotificationBodyValidation to include twilio and require a Twilio phone number with explicit error messages. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant NotificationService
participant TwilioService
participant SettingsService
participant TwilioAPI as Twilio API
Client->>NotificationService: sendNotification(type:"twilio", address, content)
alt type == "twilio"
NotificationService->>TwilioService: sendSms(address, content)
rect rgb(245,247,255)
Note over TwilioService: ensure initialized & load creds
TwilioService->>SettingsService: getSettings()
SettingsService-->>TwilioService: {twilioAccountSid, twilioAuthToken, twilioPhoneNumber}
TwilioService->>TwilioService: init client
end
rect rgb(245,255,245)
Note over TwilioService: send SMS via Twilio API
TwilioService->>TwilioAPI: messages.create({to: address, from: twilioPhoneNumber, body: content})
TwilioAPI-->>TwilioService: messageSid / error
end
alt success
TwilioService-->>NotificationService: true (messageSid)
NotificationService-->>Client: success
else failure
TwilioService-->>NotificationService: false
NotificationService-->>Client: failure
end
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
🐇 I hopped through code to add a gentle ring,
SID and token tucked, a tiny phone to bring,
I check the number before the message flies,
Logs whisper success while the SMS replies. ✨📱
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title clearly and specifically describes the main change: adding Twilio SMS notification support, which aligns with the comprehensive changeset across client and server components. |
| Description check | ✅ Passed | The PR description follows the required template with all checklist items checked off, includes the issue number (#3095), provides a brief description of changes, and includes UI screenshots demonstrating the feature. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/Components/v1/NotificationIntegrationModal/Hooks/useNotification.js (1)
7-12: Add TWILIO to the local NOTIFICATION_TYPES constant.The local
NOTIFICATION_TYPESconstant is missing theTWILIOentry, which causesNOTIFICATION_TYPES.TWILIOreferenced on line 84 to beundefined. This means the new Twilio validation case will never execute.🔎 Add TWILIO to the constant
const NOTIFICATION_TYPES = { SLACK: "slack", DISCORD: "discord", TELEGRAM: "telegram", WEBHOOK: "webhook", + TWILIO: "twilio", };
🧹 Nitpick comments (5)
client/src/Pages/v1/Settings/SettingsTwilio.jsx (3)
32-32: Remove the unused comment.The comment indicates "// Local state" but no local state is defined below it. This is misleading and should be removed.
🔎 Proposed fix
} = settingsData?.settings || {}; - // Local state - if (!isAdmin) {
62-72: Fix inconsistent indentation.The Box wrapper at line 62 has extra indentation that doesn't match the surrounding code style.
🔎 Proposed fix
</Box> - <Box> - <TextInput - label={t("settingsPage.twilioSettings.labelAuthToken")} - name="twilioAuthToken" - type="password" - placeholder={t("settingsPage.twilioSettings.placeholderAuthToken")} - value={twilioAuthToken} - onChange={handleChange} - endAdornment={<PasswordEndAdornment />} - /> - </Box> + <Box> + <TextInput + label={t("settingsPage.twilioSettings.labelAuthToken")} + name="twilioAuthToken" + type="password" + placeholder={t("settingsPage.twilioSettings.placeholderAuthToken")} + value={twilioAuthToken} + onChange={handleChange} + endAdornment={<PasswordEndAdornment />} + /> + </Box>
89-95: Consider strengthening PropTypes validation.While the current PropTypes are functional, you could make them more specific by marking required props and using more precise types.
🔎 Suggested improvement
SettingsTwilio.propTypes = { - isAdmin: PropTypes.bool, + isAdmin: PropTypes.bool.isRequired, settingsData: PropTypes.object, - setSettingsData: PropTypes.func, - handleChange: PropTypes.func, + setSettingsData: PropTypes.func.isRequired, + handleChange: PropTypes.func.isRequired, HEADER_SX: PropTypes.object, };client/src/Pages/v1/Notifications/utils.js (1)
38-38: Consider using consistent key naming.The LABEL_MAP uses "twilioPhoneNumber" as the key while all other maps (TITLE_MAP, DESCRIPTION_MAP, PLACEHOLDER_MAP) use "twilio". Additionally, other notification types in LABEL_MAP use their notification type as the key (e.g., "email", "slack", "discord"). This inconsistency could cause confusion when looking up labels.
Consider whether the key should be "twilio" to match the pattern, or if there's a specific reason for using "twilioPhoneNumber".
server/src/service/v1/infrastructure/twilioService.js (1)
28-40: Consider clarifying the twilioPhoneNumber validation.Line 32 checks for
twilioPhoneNumbereven though it's not used to create the client on line 38 (onlyaccountSidandauthTokenare needed). While this is correct because the phone number is required later insendSms, consider adding a comment explaining that all three credentials must be present for the service to function, even though only two are used for client initialization.🔎 Add clarifying comment
init = async () => { const config = await this.settingsService.getDBSettings(); const { twilioAccountSid, twilioAuthToken, twilioPhoneNumber } = config; + // All three credentials are required: accountSid and authToken for client initialization, + // and phoneNumber for sending messages if (!twilioAccountSid || !twilioAuthToken || !twilioPhoneNumber) { this.logger.error({ message: "Twilio SMS notifications are disabled because the Twilio credentials are not set", service: SERVICE_NAME, }); } else { this.client = twilio(twilioAccountSid, twilioAuthToken); } };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
client/src/Components/v1/NotificationIntegrationModal/Hooks/useNotification.jsclient/src/Pages/v1/Notifications/utils.jsclient/src/Pages/v1/Settings/SettingsTwilio.jsxclient/src/Pages/v1/Settings/index.jsxclient/src/Validation/validation.jsclient/src/locales/en.jsonserver/package.jsonserver/src/config/services.jsserver/src/db/v1/models/AppSettings.jsserver/src/db/v1/models/Notification.jsserver/src/service/v1/infrastructure/notificationService.jsserver/src/service/v1/infrastructure/twilioService.jsserver/src/validation/joi.js
🧰 Additional context used
🧬 Code graph analysis (5)
client/src/Pages/v1/Settings/index.jsx (2)
client/src/Pages/v1/Settings/SettingsTwilio.jsx (2)
SettingsTwilio(14-87)settingsData(26-30)client/src/Pages/v1/Account/components/ProfilePanel.jsx (1)
handleChange(55-65)
client/src/Components/v1/NotificationIntegrationModal/Hooks/useNotification.js (1)
client/src/Pages/v1/Notifications/utils.js (2)
NOTIFICATION_TYPES(1-9)NOTIFICATION_TYPES(1-9)
client/src/Pages/v1/Settings/SettingsTwilio.jsx (2)
client/src/Components/v1/NotificationIntegrationModal/Hooks/useNotification.js (1)
useTranslation(28-28)client/src/Pages/v1/Settings/index.jsx (5)
useTranslation(78-78)theme(76-76)settingsData(45-45)isAdmin(75-75)handleChange(81-171)
server/src/service/v1/infrastructure/notificationService.js (1)
server/src/config/services.js (1)
twilioService(139-139)
server/src/service/v1/infrastructure/twilioService.js (2)
server/src/service/v1/infrastructure/notificationService.js (1)
SERVICE_NAME(1-1)server/src/index.js (1)
settingsService(24-24)
🪛 Biome (2.1.2)
client/src/Validation/validation.js
[error] 508-508: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (15)
client/src/Pages/v1/Settings/index.jsx (1)
11-11: LGTM!The integration of the SettingsTwilio component follows the established pattern used for other settings sections (e.g., SettingsEmail) and properly passes all required props.
Also applies to: 252-258
server/src/db/v1/models/Notification.js (1)
19-19: LGTM!The addition of "twilio" to the notification type enum is consistent with the existing pattern and properly extends the schema to support SMS notifications.
server/src/db/v1/models/AppSettings.js (1)
74-82: LGTM!The Twilio credential fields follow the same pattern as existing email settings. The schema appropriately defines them as optional String fields, allowing for flexible configuration.
client/src/Pages/v1/Settings/SettingsTwilio.jsx (1)
14-86: LGTM!The component follows established patterns from other settings sections and properly handles admin-only access, sensitive credential masking, and form state management.
client/src/Validation/validation.js (2)
339-341: LGTM!The Twilio settings fields are correctly added to the validation schema with appropriate string validation that allows empty values for optional configuration.
506-512: LGTM!The Twilio phone number validation correctly requires a non-empty string when the notification type is "twilio", with appropriate error messages.
client/src/locales/en.json (1)
87-89: LGTM!The localization keys are well-structured, follow established naming conventions, and provide comprehensive text for all Twilio-related UI elements and validation messages.
Also applies to: 758-762, 888-898
client/src/Pages/v1/Notifications/utils.js (2)
8-8: LGTM!The Twilio SMS notification type is correctly added with appropriate id, name, and value fields.
18-18: LGTM!The Twilio entries in TITLE_MAP, DESCRIPTION_MAP, and PLACEHOLDER_MAP correctly use "twilio" as the key and reference appropriate localization strings.
Also applies to: 28-28, 48-48
server/package.json (1)
57-57: Twilio version 5.11.1 is valid and free from known security vulnerabilities.Verification confirms that Twilio SDK version 5.11.1 exists on npm and has no known security advisories. The package name "twilio" is the official release and is properly specified.
server/src/config/services.js (1)
7-7: LGTM! Twilio service properly wired.The TwilioService import, instantiation, and injection into NotificationService follow the established pattern used for other infrastructure services like EmailService.
Also applies to: 139-139, 166-166
server/src/service/v1/infrastructure/notificationService.js (2)
7-7: LGTM! TwilioService properly integrated.The twilioService is correctly added to the constructor parameters and assigned to the instance.
Also applies to: 14-14
59-63: LGTM! Twilio notification handling follows existing patterns.The implementation for handling Twilio SMS notifications mirrors the email notification pattern, maintaining consistency in the codebase.
server/src/validation/joi.js (1)
438-440: LGTM! Twilio settings fields properly validated.The three new Twilio configuration fields are correctly added with appropriate validation (optional strings with empty string support).
server/src/service/v1/infrastructure/twilioService.js (1)
1-82: LGTM! TwilioService implementation is solid with good resilience patterns.The implementation includes:
- Proper error handling and logging throughout
- Re-initialization logic in
sendSms(lines 50-55) to handle cases where credentials are added after startup- Graceful failure by returning
falseinstead of throwing errors- Fresh settings fetch in
sendSms(line 56) to catch configuration updatesThe async initialization in the constructor (line 25) without
awaitis handled correctly by the re-initialization check insendSms.
![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 adds Twilio SMS notification support but introduces a critical security vulnerability by logging sensitive SMS content and PII, alongside several maintenance and consistency issues.
📄 Documentation Diagram
This diagram documents the new Twilio SMS notification integration workflow.
sequenceDiagram
participant U as User
participant NS as NotificationService
participant TS as TwilioService
participant L as Logger
note over U,TS: PR #35;3096 adds Twilio SMS support
U->>NS: Trigger notification
NS->>TS: sendSms(to, text)
TS->>L: Log sending action
TS->>TS: Send SMS via Twilio API
TS-->>NS: Return success/failure
NS-->>U: Notification result
| Priority | File | Category | Impact Summary (≤12 words) | Anchors |
|---|---|---|---|---|
| P1 | server/.../twilioService.js | Security | Logs sensitive SMS content and PII, risking unauthorized access. | |
| P2 | server/.../twilioService.js | Architecture | Async init in constructor may cause race conditions and duplicate initialization. | |
| P2 | server/src/validation/joi.js | Maintainability | Missing phone number format validation leads to silent notification failures. | path:server/src/db/v1/models/Notification.js |
| P2 | client/src/Validation/validation.js | Bug | Incomplete error message omits valid notification types, confusing users. | path:server/src/validation/joi.js |
| P2 | client/.../SettingsTwilio.jsx | Architecture | Insecure handling of auth token compared to email settings, risking leakage. |
🔍 Notable Themes
- Security Logging: Multiple findings highlight risks in logging sensitive data and handling authentication tokens.
- Validation Consistency: Gaps in input validation across client and server could lead to user confusion and system failures.
📈 Risk Diagram
This diagram illustrates the risk of logging sensitive data in the Twilio SMS sending process.
sequenceDiagram
participant NS as NotificationService
participant TS as TwilioService
participant L as Logger
NS->>TS: sendSms(to, text)
TS->>L: log SMS details
note over L: R1(P1): Logs sensitive SMS content and PII
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: server/src/validation/joi.js
The server-side validation for the Twilio notification's address field (which stores the phone number) only checks for a non-empty string but does not validate its format as a phone number (e.g., E.164 format: +1234567890). This allows malformed numbers to be saved to the database (Notification model). When twilioService.sendSms is called with an invalid number, the Twilio API will reject it, causing a silent notification failure that is only logged as an error. This degrades user experience and makes debugging harder. The related context shows the Notification model's address field is a string, so this issue directly impacts data quality and system reliability.
Suggestion:
{
is: "twilio",
then: joi.string().pattern(/^\+[1-9]\d{1,14}$/).required().messages({
"string.empty": "Twilio phone number cannot be empty",
"any.required": "Twilio phone number is required",
"string.pattern.base": "Phone number must be in E.164 format (e.g., +1234567890)",
}),
}Related Code:
{
is: "twilio",
then: joi.string().required().messages({
"string.empty": "Twilio phone number cannot be empty",
"any.required": "Twilio phone number is required",
}),
}💡 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.
| this.logger.info({ | ||
| message: `Sending Twilio SMS to ${to}: ${text}`, | ||
| service: SERVICE_NAME, | ||
| }); |
There was a problem hiding this comment.
P1 | Confidence: High
The sendSms method logs the full SMS body (text) and recipient phone number (to) in plaintext. This creates a security vulnerability as SMS messages often contain sensitive alert details (e.g., server names, incident details, IPs) and phone numbers are personally identifiable information (PII). Storing this data in logs exposes it to unauthorized access and violates the principle of least privilege. The impact is immediate as this occurs on every notification.
| this.logger.info({ | |
| message: `Sending Twilio SMS to ${to}: ${text}`, | |
| service: SERVICE_NAME, | |
| }); | |
| this.logger.info({ | |
| message: `Sending Twilio SMS to ${to.slice(0, 4)}... via configured sender`, | |
| service: SERVICE_NAME, | |
| }); |
| type: joi | ||
| .string() | ||
| .valid("email", "webhook", "slack", "discord", "pager_duty", "matrix") | ||
| .valid("email", "webhook", "slack", "discord", "pager_duty", "matrix", "twilio") | ||
| .required() | ||
| .messages({ | ||
| "string.empty": "Notification type is required", | ||
| "any.required": "Notification type is required", | ||
| "any.only": "Notification type must be email, webhook, or pager_duty", | ||
| "any.only": "Notification type must be email, webhook, pager_duty, matrix, or twilio", | ||
| }), |
There was a problem hiding this comment.
P2 | Confidence: High
The client-side validation error message for the type field is incomplete and misleading. It lists email, webhook, pager_duty, matrix, or twilio but omits slack and discord, which are valid values in the .valid() list. This inconsistency creates a poor user experience, as a user selecting slack could receive an error message implying it's not supported. The related server-side validation correctly lists all types. This is a functional bug in user-facing validation logic.
| type: joi | |
| .string() | |
| .valid("email", "webhook", "slack", "discord", "pager_duty", "matrix") | |
| .valid("email", "webhook", "slack", "discord", "pager_duty", "matrix", "twilio") | |
| .required() | |
| .messages({ | |
| "string.empty": "Notification type is required", | |
| "any.required": "Notification type is required", | |
| "any.only": "Notification type must be email, webhook, or pager_duty", | |
| "any.only": "Notification type must be email, webhook, pager_duty, matrix, or twilio", | |
| }), | |
| "any.only": "Notification type must be email, webhook, slack, discord, pager_duty, matrix, or twilio", |
Evidence: path:server/src/validation/joi.js
| const { | ||
| twilioAccountSid = "", | ||
| twilioAuthToken = "", | ||
| twilioPhoneNumber = "", | ||
| } = settingsData?.settings || {}; |
There was a problem hiding this comment.
P2 | Confidence: High
The new SettingsTwilio component does not follow the same secure handling pattern used for the analogous SettingsEmail component regarding the sensitive twilioAuthToken. In SettingsEmail, the password field uses a dedicated labelAuthTokenSet when a token is already stored and provides a "Reset" workflow to change it, preventing the current token from being displayed or transmitted unnecessarily. SettingsTwilio exposes the twilioAuthToken in a plain text field (albeit type="password"), which could lead to accidental exposure or unnecessary transmission of the secret. This introduces a security inconsistency and increases the risk of credential leakage.
| * @param {Object} settingsService - The settings service to get SMS configuration. | ||
| * @param {Object} logger - The logger module. | ||
| */ | ||
| constructor(settingsService, logger) { |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The init method is called in the constructor, but it's asynchronous and accesses settingsService.getDBSettings() without awaiting. This means the constructor returns immediately, and the Twilio client may not be initialized when the first SMS is attempted. While sendSms calls init again if the client is null, this creates a race condition. If the first notification attempt occurs before the initial init resolves, sendSms will call init again, potentially leading to duplicate initialization logic and multiple getDBSettings calls. This pattern deviates from the established synchronous dependency injection pattern used by other services (e.g., EmailService), which could lead to subtle timing bugs.
Code Suggestion:
constructor(settingsService, logger) {
this.settingsService = settingsService;
this.logger = logger;
this.client = null;
// Do not call async init here. Rely on lazy initialization in sendSms.
}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx (3)
69-128: Add Twilio configuration to DEFAULT_NOTIFICATION_TYPES array.The DEFAULT_NOTIFICATION_TYPES array is missing the Twilio entry. Following the existing pattern, a Twilio configuration should be added here.
🔎 Proposed addition for Twilio
Add this entry after the Webhook configuration (after line 127):
}, + { + id: NOTIFICATION_TYPES.TWILIO, + label: t("notifications.twilio.label"), + description: t("notifications.twilio.description"), + fields: [ + { + id: "accountSid", + label: t("notifications.twilio.accountSidLabel"), + placeholder: t("notifications.twilio.accountSidPlaceholder"), + type: "text", + }, + { + id: "authToken", + label: t("notifications.twilio.authTokenLabel"), + placeholder: t("notifications.twilio.authTokenPlaceholder"), + type: "password", + }, + { + id: "fromPhoneNumber", + label: t("notifications.twilio.fromPhoneNumberLabel"), + placeholder: t("notifications.twilio.fromPhoneNumberPlaceholder"), + type: "text", + }, + { + id: "toPhoneNumber", + label: t("notifications.twilio.toPhoneNumberLabel"), + placeholder: t("notifications.twilio.toPhoneNumberPlaceholder"), + type: "text", + }, + ], + }, ];Also update FIELD_IDS constant (after line 35):
CHAT_ID: "chatId", URL: "url", + ACCOUNT_SID: "accountSid", + AUTH_TOKEN: "authToken", + FROM_PHONE_NUMBER: "fromPhoneNumber", + TO_PHONE_NUMBER: "toPhoneNumber", };
170-193: Add Twilio case to extractNotificationValues switch statement.The switch statement doesn't handle the
NOTIFICATION_TYPES.TWILIOcase, preventing the modal from loading existing Twilio notification configurations for editing.🔎 Proposed fix to add Twilio case
Add this case before the WEBHOOK case (after line 187):
break; + case NOTIFICATION_TYPES.TWILIO: + if (notification.config.accountSid) { + values[getFieldKey(platform, "accountSid")] = + notification.config.accountSid; + } + if (notification.config.authToken) { + values[getFieldKey(platform, "authToken")] = + notification.config.authToken; + } + if (notification.config.fromPhoneNumber) { + values[getFieldKey(platform, "fromPhoneNumber")] = + notification.config.fromPhoneNumber; + } + if (notification.config.toPhoneNumber) { + values[getFieldKey(platform, "toPhoneNumber")] = + notification.config.toPhoneNumber; + } + break; case NOTIFICATION_TYPES.WEBHOOK:
275-291: Add Twilio case to handleSave switch statement.The switch statement doesn't handle the
"twilio"case, preventing the modal from saving Twilio notification configurations.🔎 Proposed fix to add Twilio case
Add this case before the "webhook" case (after line 286):
break; + case "twilio": + notificationObject.config.accountSid = + integrations[getFieldKey(type.id, "accountSid")]; + notificationObject.config.authToken = + integrations[getFieldKey(type.id, "authToken")]; + notificationObject.config.fromPhoneNumber = + integrations[getFieldKey(type.id, "fromPhoneNumber")]; + notificationObject.config.toPhoneNumber = + integrations[getFieldKey(type.id, "toPhoneNumber")]; + break; case "webhook":
♻️ Duplicate comments (1)
server/src/validation/joi.js (1)
618-624: Add phone number format validation for Twilio.The required validation was added (good!), but phone number format validation is still missing. Without format validation, invalid phone numbers can be stored and will cause Twilio API failures at runtime when notifications are sent.
Consider validating against E.164 format (international phone number standard) to catch errors early and improve the user experience.
🔎 Suggested fix: Add E.164 phone number pattern validation
{ is: "twilio", - then: joi.string().required().messages({ + then: joi.string() + .pattern(/^\+?[1-9]\d{1,14}$/) + .required() + .messages({ "string.empty": "Twilio phone number cannot be empty", "any.required": "Twilio phone number is required", + "string.pattern.base": "Please enter a valid phone number in E.164 format (e.g., +1234567890)", }), },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsxclient/src/Pages/v1/Notifications/utils.jsclient/src/Pages/v1/Settings/SettingsTwilio.jsxserver/src/validation/joi.js
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/Pages/v1/Notifications/utils.js
- client/src/Pages/v1/Settings/SettingsTwilio.jsx
🧰 Additional context used
🪛 Biome (2.1.2)
server/src/validation/joi.js
[error] 620-620: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (2)
server/src/validation/joi.js (2)
438-440: LGTM: Twilio settings fields are correctly defined.The three Twilio configuration fields are appropriately defined as optional strings, allowing the settings to be empty until configured by the administrator.
583-586: LGTM: Notification type correctly includes Twilio.The "twilio" notification type is properly added to the validation schema with an appropriate error message.
...t/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/Validation/validation.js (1)
506-512: LGTM! Consider optional phone number format validation.The Twilio phone number validation follows the same pattern as PagerDuty (requiring a non-empty string). The implementation is correct and consistent.
For improved user experience, you could optionally add format validation to catch invalid phone numbers early (e.g., E.164 format like
+1234567890), though this may not be necessary if the Twilio SDK handles validation on the backend.Optional: Add phone number format validation
{ is: "twilio", - then: joi.string().required().messages({ - "string.empty": "Twilio phone number cannot be empty", - "any.required": "Twilio phone number is required", - }), + then: joi.string() + .pattern(/^\+[1-9]\d{1,14}$/) + .required() + .messages({ + "string.empty": "Twilio phone number cannot be empty", + "any.required": "Twilio phone number is required", + "string.pattern.base": "Please enter a valid phone number in E.164 format (e.g., +1234567890)", + }), },Note: The static analysis warning about the
thenproperty is a false positive—thenis part of Joi's conditional validation API and is used correctly throughout this file.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/Validation/validation.js
🧰 Additional context used
🪛 Biome (2.1.2)
client/src/Validation/validation.js
[error] 508-508: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (2)
client/src/Validation/validation.js (2)
339-341: LGTM! Twilio settings follow the established pattern.The three Twilio credential fields are correctly defined as optional strings, consistent with how other optional credentials (like email settings) are handled in this validation schema.
465-470: Excellent! Previous feedback addressed.The error message now correctly lists all valid notification types, including "slack" and "discord" that were previously missing. The message now matches the enum exactly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/Validation/validation.js (1)
507-513: Consider adding phone number format validation.The current validation only ensures the phone number is non-empty. Adding E.164 format validation would catch invalid phone numbers early and improve user experience.
Note: The Biome linter flags the
thenproperty (line 509), but this is a false positive —thenis the standard Joi API for conditional validation inswitchclauses, not a Promise-related property.♻️ Optional: Add E.164 phone number format validation
{ is: "twilio", - then: joi.string().required().messages({ + then: joi.string().pattern(/^\+[1-9]\d{1,14}$/).required().messages({ "string.empty": "Twilio phone number cannot be empty", "any.required": "Twilio phone number is required", + "string.pattern.base": "Please enter a valid phone number in E.164 format (e.g., +12125551234)", }), },client/src/Pages/v1/Settings/SettingsTwilio.jsx (1)
12-18: UnusedsetSettingsDataprop.The
setSettingsDataprop is destructured but never used in this component. Based on thehandleChangeimplementation in the parentindex.jsx, state updates are already handled there.Consider removing it from the props if it's not needed, or document why it's passed if there's an intentional reason.
♻️ Suggested fix
const SettingsTwilio = ({ isAdmin, HEADER_SX, handleChange, settingsData, - setSettingsData, }) => {And update PropTypes accordingly:
SettingsTwilio.propTypes = { isAdmin: PropTypes.bool, settingsData: PropTypes.object, - setSettingsData: PropTypes.func, handleChange: PropTypes.func, HEADER_SX: PropTypes.object, };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Pages/v1/Settings/SettingsTwilio.jsxclient/src/Validation/validation.js
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Pages/v1/Settings/SettingsTwilio.jsx (1)
client/src/Pages/v1/Settings/index.jsx (1)
handleChange(81-171)
🪛 Biome (2.1.2)
client/src/Validation/validation.js
[error] 509-509: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (4)
client/src/Validation/validation.js (2)
339-341: LGTM - Twilio settings fields added correctly.The optional fields with
allow("")are appropriate for configuration that may not always be present. For enhanced validation, you could optionally add format checks (e.g., Account SID typically starts with "AC"), but this is fine for an initial implementation since server-side validation should catch invalid credentials.
463-472: LGTM - Type validation correctly extended.The addition of "twilio" to valid types and the updated error message are consistent with the existing pattern.
client/src/Pages/v1/Settings/SettingsTwilio.jsx (2)
34-82: LGTM!The component structure is clean and follows existing patterns:
- Proper i18n usage for all user-facing strings
- Auth token field correctly uses
type="password"withPasswordEndAdornmentfor secure input handling- Defensive defaults prevent undefined value issues
- Admin-only rendering is correctly implemented
85-91: PropTypes are adequate.The prop types cover all declared props. The use of generic
PropTypes.objectforsettingsDatais acceptable here since the component defensively handles missing nested properties.
…ilio configuration to DEFAULT_NOTIFICATION_TYPES array.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx:
- Around line 128-140: Add persistent save/load support for Twilio by (1) adding
PHONE_NUMBER: "phoneNumber" to the FIELD_IDS constant, (2) ensuring the Twilio
field in the notification type uses FIELD_IDS.PHONE_NUMBER for its id, (3)
adding a NOTIFICATION_TYPES.TWILIO case to extractNotificationValues to set
values.phoneNumber from notificationObject.config.phoneNumber, and (4) adding a
NOTIFICATION_TYPES.TWILIO case to handleSave to populate
notificationObject.config.phoneNumber from values.phoneNumber (also confirm that
notificationObject.type for Twilio should be NOTIFICATION_TYPES.TWILIO and not
"webhook" to match server expectations).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsxclient/src/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/locales/en.json
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx (1)
client/src/Pages/v1/Notifications/utils.js (2)
NOTIFICATION_TYPES(1-9)NOTIFICATION_TYPES(1-9)
🔇 Additional comments (1)
client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx (1)
27-27: LGTM!The TWILIO type constant is correctly added and matches the value used in
utils.js.
| { | ||
| id: NOTIFICATION_TYPES.TWILIO, | ||
| label: t("notifications.twilio.label"), | ||
| description: t("notifications.twilio.description"), | ||
| fields: [ | ||
| { | ||
| id: "phoneNumber", | ||
| label: t("notifications.twilio.phoneNumberLabel"), | ||
| placeholder: t("notifications.twilio.phoneNumberPlaceholder"), | ||
| type: "text", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Missing save and load logic for Twilio notifications.
The Twilio notification type is added to the UI, but the implementation is incomplete:
handleSave(lines 288-304): The switch statement doesn't handle"twilio", sonotificationObject.configwill be empty when saved.extractNotificationValues(lines 183-206): The switch statement doesn't handleNOTIFICATION_TYPES.TWILIO, so existing Twilio notifications won't populate the phoneNumber field when the modal opens.FIELD_IDSconstant: For consistency, addPHONE_NUMBER: "phoneNumber"to theFIELD_IDSobject.
Proposed fix
Add to FIELD_IDS constant (around line 36):
const FIELD_IDS = {
WEBHOOK: "webhook",
TOKEN: "token",
CHAT_ID: "chatId",
URL: "url",
+ PHONE_NUMBER: "phoneNumber",
};Update the Twilio field definition (line 134):
- id: "phoneNumber",
+ id: FIELD_IDS.PHONE_NUMBER,Add Twilio case to extractNotificationValues switch (after line 206):
case NOTIFICATION_TYPES.WEBHOOK:
if (notification.config.webhookUrl) {
values[getFieldKey(platform, FIELD_IDS.URL)] =
notification.config.webhookUrl;
}
break;
+ case NOTIFICATION_TYPES.TWILIO:
+ if (notification.config.phoneNumber) {
+ values[getFieldKey(platform, FIELD_IDS.PHONE_NUMBER)] =
+ notification.config.phoneNumber;
+ }
+ break;
}Add Twilio case to handleSave switch (after line 303):
case "webhook":
notificationObject.config.webhookUrl =
integrations[getFieldKey(type.id, "url")];
break;
+ case "twilio":
+ notificationObject.type = "twilio";
+ notificationObject.config.phoneNumber =
+ integrations[getFieldKey(type.id, FIELD_IDS.PHONE_NUMBER)];
+ break;
}Note: Verify whether notificationObject.type should be "twilio" instead of "webhook" for Twilio notifications based on server-side expectations.
📝 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.
| { | |
| id: NOTIFICATION_TYPES.TWILIO, | |
| label: t("notifications.twilio.label"), | |
| description: t("notifications.twilio.description"), | |
| fields: [ | |
| { | |
| id: "phoneNumber", | |
| label: t("notifications.twilio.phoneNumberLabel"), | |
| placeholder: t("notifications.twilio.phoneNumberPlaceholder"), | |
| type: "text", | |
| }, | |
| ], | |
| }, | |
| { | |
| id: NOTIFICATION_TYPES.TWILIO, | |
| label: t("notifications.twilio.label"), | |
| description: t("notifications.twilio.description"), | |
| fields: [ | |
| { | |
| id: FIELD_IDS.PHONE_NUMBER, | |
| label: t("notifications.twilio.phoneNumberLabel"), | |
| placeholder: t("notifications.twilio.phoneNumberPlaceholder"), | |
| type: "text", | |
| }, | |
| ], | |
| }, |
🤖 Prompt for AI Agents
In
@client/src/Components/v1/NotificationIntegrationModal/Components/NotificationIntegrationModal.jsx
around lines 128 - 140, Add persistent save/load support for Twilio by (1)
adding PHONE_NUMBER: "phoneNumber" to the FIELD_IDS constant, (2) ensuring the
Twilio field in the notification type uses FIELD_IDS.PHONE_NUMBER for its id,
(3) adding a NOTIFICATION_TYPES.TWILIO case to extractNotificationValues to set
values.phoneNumber from notificationObject.config.phoneNumber, and (4) adding a
NOTIFICATION_TYPES.TWILIO case to handleSave to populate
notificationObject.config.phoneNumber from values.phoneNumber (also confirm that
notificationObject.type for Twilio should be NOTIFICATION_TYPES.TWILIO and not
"webhook" to match server expectations).
1 participant
Describe your changes
Add Twilio SMS Notification Support
For feature #3095
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.