Add Discord webhook notification support #1612
Conversation
|
Warning Rate limit exceeded@Skorpios604 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request expands the notification system by updating the Changes
Sequence DiagramsequenceDiagram
participant Service as NotificationService
participant Discord as Discord Webhook
participant Slack as Slack Webhook
participant Telegram as Telegram Webhook
participant Logger as Error Logger
Service->>Discord: POST notification message
alt Successful Discord notification
Discord-->>Service: Successful response
else Discord notification failed
Service->>Logger: Log Discord error details
end
Service->>Slack: POST notification message
alt Successful Slack notification
Slack-->>Service: Successful response
else Slack notification failed
Service->>Logger: Log Slack error details
end
Service->>Telegram: POST notification message
alt Successful Telegram notification
Telegram-->>Service: Successful response
else Telegram notification failed
Service->>Logger: Log Telegram error details
end
The sequence diagram illustrates the process of sending notifications to Discord, Slack, and Telegram, including success and error handling paths for each communication channel. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Server/service/notificationService.js (1)
Line range hint
91-102: There's vomit on his sweater - we forgot to handle Discord notifications! 🍝The handleStatusNotifications method doesn't handle the new "discord" notification type.
Add Discord support to the notification handler:
for (const notification of notifications) { if (notification.type === "email") { this.sendEmail(networkResponse, notification.address); } - // Handle other types of notifications here + else if (notification.type === "discord") { + await this.sendDiscordNotification(networkResponse, notification.webhookUrl); + } }
🧹 Nitpick comments (1)
Server/service/notificationService.js (1)
20-38: His palms are sweaty - we need some tests! 🍝The new Discord notification functionality lacks test coverage.
Would you like me to generate unit tests for the Discord notification functionality? The tests would cover:
- Webhook URL validation
- Message formatting
- Rate limiting handling
- Error scenarios
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/db/models/Notification.js(1 hunks)Server/service/notificationService.js(2 hunks)
Server/db/models/Notification.js
Outdated
| @@ -8,7 +8,7 @@ const NotificationSchema = mongoose.Schema( | |||
| }, | |||
| type: { | |||
| type: String, | |||
| enum: ["email", "sms"], | |||
| enum: ["email", "sms", "discord"], | |||
There was a problem hiding this comment.
Yo dawg, we need a field for that webhook URL! 🍝
While adding "discord" to the enum is a good start, we're missing a dedicated field to store the Discord webhook URL. The existing address field could be misused for this, but that's not clean architecture.
Add a new field to properly store webhook URLs:
type: {
type: String,
enum: ["email", "sms", "discord"],
},
+webhookUrl: {
+ type: String,
+ validate: {
+ validator: function(v) {
+ return this.type !== "discord" ||
+ /^https:\/\/discord\.com\/api\/webhooks\/[0-9]+\/.*$/.test(v);
+ },
+ message: "Invalid Discord webhook URL format"
+ }
+},Committable suggestion skipped: line range outside the PR's diff.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 PR Summary
- Business value and requirements alignment: This PR introduces Discord webhook notification support, enhancing the existing monitoring system by adding a new notification channel. This aligns with the business requirement of expanding user notification options beyond email and SMS, thereby improving user experience and engagement.
- Key components modified: The PR modifies the
Notificationmodel to include Discord as a notification type and adds a new functionsendDiscordNotificationin thenotificationServiceto handle Discord notifications. - Impact assessment: The addition of Discord notifications introduces a new external dependency (Discord API) and requires handling asynchronous operations and potential network failures. This impacts the system's architecture by adding a new notification channel and integrating with an external service.
- System dependencies and integration impacts: The new feature depends on the Discord API for sending notifications. Proper error handling and fallback mechanisms are crucial to maintain system stability. The integration also introduces security considerations, such as securely managing webhook URLs.
1.2 Architecture Changes
- System design modifications: The system design is modified to include Discord as a notification channel. This involves changes to the
Notificationmodel and the addition of a new function in thenotificationService. - Component interactions: The
notificationServicenow interacts with the Discord API to send notifications. This introduces a new external dependency and requires handling asynchronous operations. - Integration points: The integration point is the
sendDiscordNotificationfunction, which makes outbound HTTP requests to the Discord API.
2. Detailed Technical Analysis
2.1 Code Logic Deep-Dive
Core Logic Changes
- Server/service/notificationService.js - sendDiscordNotification
- Submitted PR Code:
async sendDiscordNotification(networkResponse, webhookUrl) {
const { monitor, status } = networkResponse;
const message = {
content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`
};
try {
await axios.post(webhookUrl, message);
return true;
} catch (error) {
this.logger.error({
message: error.message,
service: this.SERVICE_NAME,
method: "sendDiscordNotification",
stack: error.stack,
});
return false;
}
}- Analysis:
- The current error handling in
sendDiscordNotificationis basic. It catches any error during theaxios.postcall, logs it, and returnsfalse. - It lacks specific error handling based on the type of error. For example, Discord API might return different status codes for various issues like invalid webhook URL (400), rate limiting (429), or server errors (5xx).
- There is no retry mechanism for transient network errors or temporary Discord API issues. Simply logging and returning
falsemight lead to missed notifications even if the issue is temporary. - The error log message includes
error.messageanderror.stack, which is good for debugging, but it could be enhanced with more context, such as thewebhookUrlbeing used (obfuscated for security in production logs, but helpful in development/debugging) and themonitor.nameto easily identify the source of the notification failure.
- The current error handling in
- LlamaPReview Suggested Improvements:
async sendDiscordNotification(networkResponse, webhookUrl) {
const { monitor, status } = networkResponse;
const message = {
content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`
};
try {
await axios.post(webhookUrl, message);
return true;
} catch (error) {
this.logger.error({
message: `Discord notification failed for monitor: ${monitor.name}, webhookUrl: ${webhookUrl.substring(0, 20)}... Error: ${error.message}`, // Improved logging with monitor name and partial webhook URL
service: this.SERVICE_NAME,
method: "sendDiscordNotification",
stack: error.stack,
});
// Consider adding retry logic here for transient errors, potentially with exponential backoff.
// Example (basic retry - for production, use a more robust library/strategy):
// if (error.response && error.response.status >= 500) {
// await new Promise(resolve => setTimeout(resolve, 1000)); // Wait 1 second before retry
// try {
// await axios.post(webhookUrl, message);
// return true; // Retry success
// } catch (retryError) {
// this.logger.error({ ...retryErrorLog }); // Log retry error
// return false; // Retry failed
// }
// }
return false;
}
}- Improvement rationale:
- Technical benefits:
- Improved error logging: Provides more context for debugging notification failures, including the monitor name and a truncated webhook URL for identification without exposing the full secret.
- Foundation for retry logic: The suggested code includes comments and a basic example of where retry logic could be implemented. This addresses the potential for transient network errors and improves the reliability of notifications. A more robust retry mechanism (using libraries like
axios-retryor implementing exponential backoff) would be beneficial in a production environment.
- Business value:
- Increased notification reliability: Retry logic, even basic, can significantly reduce missed notifications due to temporary issues, leading to more reliable alerting for users.
- Risk assessment:
- Low risk: The logging improvement is a low-risk change. Adding retry logic introduces slightly more complexity but can be implemented incrementally and tested thoroughly. Not implementing retry logic carries the risk of unreliable notifications.
- Technical benefits:
Core Logic Changes
- Server/service/notificationService.js - sendDiscordNotification
- Submitted PR Code:
async sendDiscordNotification(networkResponse, webhookUrl) {
const { monitor, status } = networkResponse;
const message = {
content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`
};
try {
await axios.post(webhookUrl, message);
return true;
} catch (error) {
this.logger.error({
message: error.message,
service: this.SERVICE_NAME,
method: "sendDiscordNotification",
stack: error.stack,
});
return false;
}
}- Analysis:
- The
sendDiscordNotificationfunction directly takeswebhookUrlas a parameter. This implies that the webhook URL is being passed to this function from elsewhere in the application. - The PR does not show where this
webhookUrloriginates from, how it is stored, or how it is managed. This is a significant security and operational gap. - Security Implications: If webhook URLs are not handled securely, they could be exposed, leading to unauthorized access or misuse of the Discord webhook. Hardcoding webhook URLs is a major security vulnerability and scalability issue. Storing them in plain text in a database or configuration file without proper access control is also risky.
- Missing Input Validation and Sanitization: The code does not show any validation or sanitization of the
webhookUrlbefore using it in theaxios.postrequest. It's crucial to validate that the URL is a valid webhook URL format and potentially sanitize it to prevent injection attacks (though less likely in this context, validation is still important).
- The
- LlamaPReview Suggested Improvements: [Improvement focuses on highlighting the missing aspects, as the PR doesn't include the webhook URL management part]
// ... (rest of the code remains the same as the submitted PR code for this function)
// --- CRITICAL SECURITY & OPERATIONAL CONSIDERATIONS (NOT INCLUDED IN PR CODE SNIPPET) ---
// 1. Webhook URL Source and Storage:
// - Where does the 'webhookUrl' parameter come from?
// - How are webhook URLs stored? (e.g., database, configuration file, secrets manager)
// - Are webhook URLs encrypted at rest?
// - Is access to webhook URLs controlled and restricted?
// 2. Webhook URL Management:
// - Is there a UI or API for users to add/manage their Discord webhook URLs?
// - How are webhook URLs associated with monitors or notification configurations?
// 3. Input Validation and Sanitization:
// - Before calling sendDiscordNotification, the application MUST validate and sanitize the webhookUrl.
// - Validate URL format to ensure it's a valid URL.
// - Sanitize to prevent any potential injection risks (though less likely with webhook URLs, defense in depth is good practice).
// 4. Security Best Practices:
// - Treat webhook URLs as sensitive secrets.
// - Avoid logging full webhook URLs in production logs.
// - Consider using environment variables or a secrets management system to store webhook URLs securely.
async sendDiscordNotification(networkResponse, webhookUrl) { // webhookUrl is passed in - its origin and security are critical missing pieces
// ... (function code as in submitted PR) ...
}- Improvement rationale:
- Technical benefits:
- Highlights critical security gaps: Explicitly points out the missing aspects of webhook URL management, storage, and security, which are essential for a production-ready feature.
- Emphasizes security best practices: Recommends treating webhook URLs as secrets, secure storage, and input validation, aligning with industry security standards.
- Business value:
- Prevents security vulnerabilities: Addressing webhook URL security is crucial to prevent potential misuse and data breaches, protecting user data and the application's integrity.
- Ensures operational robustness: Proper management and storage of webhook URLs are necessary for the long-term maintainability and scalability of the Discord notification feature.
- Risk assessment:
- High risk if not addressed: Failure to secure webhook URLs poses a significant security risk. The suggested improvements are crucial to mitigate this high risk. The PR, in its current form, is incomplete from a security perspective regarding secret management.
- Technical benefits:
2.2 Implementation Quality
- Code organization and structure: The code is well-organized, with the new
sendDiscordNotificationfunction clearly defined within thenotificationService. The addition ofaxiosfor HTTP requests is appropriate for the task. - Design patterns usage: The use of a service class (
NotificationService) to handle notifications is a good design pattern, promoting separation of concerns and maintainability. - Error handling approach: The current error handling is basic but can be improved with more specific error handling and retry logic, as suggested in the deep-dive analysis.
- Resource management: The code efficiently manages resources by using asynchronous operations with
axiosfor HTTP requests.
3. Critical Findings
3.1 Potential Issues
-
🔴 Critical Issues
- Webhook URL Management: The PR does not address how webhook URLs are managed, stored, or validated. This poses a significant security risk, as improper handling of webhook URLs can lead to unauthorized access or misuse.
- Impact: Potential security vulnerabilities and operational issues.
- Recommendation: Implement secure storage, management, and validation of webhook URLs. Treat them as sensitive secrets and follow security best practices.
- Webhook URL Management: The PR does not address how webhook URLs are managed, stored, or validated. This poses a significant security risk, as improper handling of webhook URLs can lead to unauthorized access or misuse.
-
🟡 Warnings
- Basic Error Handling: The current error handling in
sendDiscordNotificationis basic and lacks specific handling for different types of errors.- Potential risks: Missed notifications due to temporary issues, lack of context in error logs for debugging.
- Suggested improvements: Enhance error logging with more context, such as the monitor name and a truncated webhook URL. Consider implementing retry logic for transient errors.
- Basic Error Handling: The current error handling in
3.2 Code Quality Concerns
- Maintainability aspects: The code is maintainable, but the lack of specific error handling and retry logic could lead to maintenance challenges in the future.
- Readability issues: The code is generally readable, but adding more context to error logs and implementing retry logic would improve readability and debuggability.
- Performance bottlenecks: The current implementation does not indicate any significant performance bottlenecks. However, adding retry logic should consider performance implications, such as exponential backoff to avoid overwhelming the Discord API.
4. Security Assessment
- Authentication/Authorization impacts: The PR does not introduce changes to authentication or authorization mechanisms. However, ensuring that only authorized users can manage webhook URLs is crucial.
- Data handling concerns: Webhook URLs must be handled securely to prevent unauthorized access or misuse.
- Input validation: The PR lacks input validation for webhook URLs. It is essential to validate and sanitize webhook URLs before use.
- Security best practices: Treat webhook URLs as sensitive secrets. Avoid logging full webhook URLs in production logs. Consider using environment variables or a secrets management system to store webhook URLs securely.
- Potential security risks: Improper handling of webhook URLs poses a significant security risk.
- Mitigation strategies: Implement secure storage, management, and validation of webhook URLs. Follow security best practices for handling sensitive secrets.
- Security testing requirements: Conduct security testing to ensure that webhook URLs are handled securely and that the Discord integration does not introduce vulnerabilities.
5. Testing Strategy
5.1 Test Coverage
- Unit test analysis: Ensure that the
sendDiscordNotificationfunction is covered by unit tests, including tests for successful notifications and various error scenarios. - Integration test requirements: Implement integration tests to confirm end-to-end Discord notification delivery. Include tests that simulate Discord API failures and network outages.
- Edge cases coverage: Cover edge cases such as invalid webhook URLs, rate limiting by the Discord API, and temporary network issues.
5.2 Test Recommendations
Suggested Test Cases
// Unit test for successful Discord notification
it('should send Discord notification successfully', async () => {
const networkResponse = { monitor: { name: 'Test Monitor', url: 'http://example.com' }, status: true };
const webhookUrl = 'https://discord.com/api/webhooks/...';
const mockAxios = jest.spyOn(axios, 'post').mockResolvedValueOnce({});
const result = await notificationService.sendDiscordNotification(networkResponse, webhookUrl);
expect(result).toBe(true);
expect(mockAxios).toHaveBeenCalledWith(webhookUrl, {
content: `Monitor Test Monitor is up. URL: http://example.com`
});
});
// Unit test for Discord notification failure
it('should handle Discord notification failure and log error', async () => {
const networkResponse = { monitor: { name: 'Test Monitor', url: 'http://example.com' }, status: false };
const webhookUrl = 'https://discord.com/api/webhooks/...';
const mockAxios = jest.spyOn(axios, 'post').mockRejectedValueOnce(new Error('Test error'));
const mockLogger = jest.spyOn(notificationService.logger, 'error').mockImplementationOnce(() => {});
const result = await notificationService.sendDiscordNotification(networkResponse, webhookUrl);
expect(result).toBe(false);
expect(mockAxios).toHaveBeenCalledWith(webhookUrl, {
content: `Monitor Test Monitor is down. URL: http://example.com`
});
expect(mockLogger).toHaveBeenCalledWith(expect.objectContaining({
message: expect.stringContaining('Discord notification failed for monitor: Test Monitor, webhookUrl: https://discord.com/api/webhooks/... Error: Test error')
}));
});- Coverage improvements: Ensure that tests cover all possible error scenarios, including different HTTP status codes returned by the Discord API.
- Performance testing needs: Conduct performance testing to ensure that the Discord integration can handle the expected load and that retry logic (if implemented) does not overwhelm the Discord API.
6. Documentation & Maintenance
- Documentation updates needed: Update the API documentation to include the new Discord notification feature. Provide guidelines on how to securely manage and store webhook URLs.
- Long-term maintenance considerations: Ensure that webhook URLs are managed securely and that the Discord integration is monitored for any issues. Implement robust error handling and retry logic to improve the reliability of notifications.
- Technical debt and monitoring requirements: Monitor the Discord integration for any issues and address technical debt related to error handling and retry logic.
7. Deployment & Operations
- Deployment impact and strategy: The deployment of this feature introduces a new external dependency (Discord API). Ensure that the deployment process includes testing the Discord integration in a staging environment before deploying to production.
- Key operational considerations: Monitor the Discord integration for any issues and ensure that webhook URLs are managed securely. Implement robust error handling and retry logic to improve the reliability of notifications.
8. Summary & Recommendations
8.1 Key Action Items
- Implement secure storage, management, and validation of webhook URLs.
- Enhance error logging with more context, such as the monitor name and a truncated webhook URL.
- Consider implementing retry logic for transient errors to improve the reliability of notifications.
- Conduct security testing to ensure that webhook URLs are handled securely and that the Discord integration does not introduce vulnerabilities.
8.2 Future Considerations
- Technical evolution path: Continuously improve the error handling and retry logic for the Discord integration. Explore using more robust libraries or strategies for retry logic.
- Business capability evolution: Expand the notification capabilities to include more channels and features based on user feedback and business requirements.
- System integration impacts: Ensure that the Discord integration is monitored for any issues and that it can handle the expected load. Address any technical debt related to error handling and retry logic.
💡 Help Shape LlamaPReview
How's this review format working for you? Vote in our Github Discussion Polls to help us improve your review experience!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/db/models/Notification.js(1 hunks)Server/service/notificationService.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/db/models/Notification.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
Server/service/notificationService.js (3)
2-2: Yo dawg, we need to lock down that axios version! 🍝The axios import is using a loose version in package.json. This could lead to unexpected breaks when dependencies update.
20-38: Knees weak, arms are heavy - this Discord notification needs more already! 🍝The current implementation needs improvement in several areas:
- No webhook URL validation
- Basic message format (Discord supports rich embeds)
- No rate limiting consideration
- No retry mechanism for failed requests
40-58: 🛠️ Refactor suggestionMom's spaghetti alert - this Slack notification is too basic! 🍝
The Slack implementation needs similar improvements:
async sendSlackNotification(networkResponse, webhookUrl) { + if (!webhookUrl?.startsWith('https://hooks.slack.com/services/')) { + throw new Error('Invalid Slack webhook URL'); + } + const { monitor, status } = networkResponse; const message = { - text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + blocks: [ + { + type: "section", + text: { + type: "mrkdwn", + text: `*Monitor Status Change*\n${status ? "🟢" : "🔴"} ${monitor.name}` + } + }, + { + type: "section", + fields: [ + { + type: "mrkdwn", + text: `*Status:*\n${status ? "Operational" : "Down"}` + }, + { + type: "mrkdwn", + text: `*URL:*\n${monitor.url}` + } + ] + } + ] }; try { - await axios.post(webhookUrl, message); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + + await axios.post(webhookUrl, message, { + signal: controller.signal, + headers: { 'Content-Type': 'application/json' } + }); + + clearTimeout(timeoutId); return true; } catch (error) { + if (error.response?.status === 429) { + const retryAfter = error.response.headers['retry-after'] || 5; + await new Promise(resolve => setTimeout(resolve, retryAfter * 1000)); + return this.sendSlackNotification(networkResponse, webhookUrl); + } + this.logger.error({ message: error.message, service: this.SERVICE_NAME, method: "sendSlackNotification", - stack: error.stack, + stack: error.stack, + status: error.response?.status, + data: error.response?.data }); return false; } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Great start!
We should use the NetworkService class to make network requests as it alreaedy is provisioned with an instance of Axios and all our network requests should be in the same place.
The two webhook methods are nearly identical and can be refactored into one reusable method.
I'm not sure if the errors here are meant to be swallowed or not, my guess is probably yes? Make sure you keep the flow of the application in mind, IE the the operation of making a check, parsing its data, and inserting it into the database should not be interrupted by the failure of a notification to send.
|
|
||
| try { | ||
| await axios.post(webhookUrl, message); | ||
| return true; |
There was a problem hiding this comment.
Network operations should be carried out from the NetworkService class, let's keep all our network requests in one place with consistent error handling.
| method: "sendDiscordNotification", | ||
| stack: error.stack, | ||
| }); | ||
| return false; |
There was a problem hiding this comment.
Is this error meant to be swallowed and operations meant to continue in event of error?
If not it should have method specifics added to it and rethrown to be caught by middleware
| @@ -1,4 +1,5 @@ | |||
| const SERVICE_NAME = "NotificationService"; | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
No imports in the services please, it makes testing much more difficult 👍
There shouldn't be any imports anywhere except in the root server definition really. The exception being success/error messages
| } | ||
| } | ||
|
|
||
| async sendSlackNotification(networkResponse, webhookUrl) { |
There was a problem hiding this comment.
sendSlackNotification and sendDiscordNotification are identical exept for the message as far as I can tell.
We can simplify and just simply have
sendWebHook({webHookUrl, msg, networkResponse})
All webhooks are going to be pretty much the same
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Server/service/notificationService.js (1)
40-58: 🛠️ Refactor suggestionDuplicate code making my knees weak!
This method is practically identical to sendDiscordNotification. Let's consolidate these into a single webhook method.
-async sendSlackNotification(networkResponse, webhookUrl) { - // ... duplicate code ... -} +async sendNotification(type, networkResponse, webhookUrl) { + const { monitor, status } = networkResponse; + const message = { + [type === 'slack' ? 'text' : 'content']: + `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }; + return this.networkService.sendWebhook(webhookUrl, message); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/db/models/Notification.js(1 hunks)Server/service/notificationService.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/db/models/Notification.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
Server/service/notificationService.js (2)
2-2: Yo dawg, we gotta move that import outta here!Previous feedback already mentioned this - services shouldn't have direct imports to make testing easier. Pass axios as a dependency through the constructor instead.
-import axios from 'axios'; class NotificationService { constructor(emailService, db, logger) { this.SERVICE_NAME = SERVICE_NAME; this.emailService = emailService; this.db = db; this.logger = logger; + this.httpClient = null; }
20-38:⚠️ Potential issueMom's spaghetti moment - we got some cleanup to do here!
Several issues need addressing:
- Network operations should live in NetworkService
- Silent error handling needs improvement - either propagate or handle explicitly
- Webhook logic is duplicated across notification methods
Consider consolidating webhook logic into a single method in NetworkService:
+async sendWebhook(url, message, options = {}) { + try { + const response = await this.httpClient.post(url, message, options); + return { success: true, data: response.data }; + } catch (error) { + throw new NotificationError(`Failed to send webhook: ${error.message}`, { + cause: error, + service: this.SERVICE_NAME, + method: 'sendWebhook' + }); + } +}Likely invalid or redundant comment.
| }); | ||
| return true; | ||
| } catch (error) { | ||
| return false; |
There was a problem hiding this comment.
I assume this won't be silently swallowed int the final implementation?
| text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`, | ||
| }; | ||
|
|
||
| const url = `https://api.telegram.org/bot${botToken}/sendMessage`; |
There was a problem hiding this comment.
Base URLs should be refcatored to constant vars in one place for easy maintenance
| async sendTelegramNotification(networkResponse, address) { | ||
| const { monitor, status } = networkResponse; | ||
|
|
||
| const [botToken, chatId] = address.split('|').map(part => part?.trim()); |
There was a problem hiding this comment.
This is odd, we are using the address to store a token and and ID?
Creative solution, but I think it would be better to properly store these in their own fields rather than hack something together to make use of the address field
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/service/notificationService.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
Server/service/notificationService.js (2)
2-2: Yo dawg, we gotta move these network ops to NetworkService! 🍝Previous feedback already highlighted this - services shouldn't have direct imports. Let's keep all network operations in NetworkService for consistent error handling and easier testing.
108-113:⚠️ Potential issueThere's vomit on the sweater already - these methods don't exist! 🍝
The code references
sendDiscordNotification,sendSlackNotification, andsendTelegramNotification, but these methods aren't implemented. Should be using the newsendWebhookNotificationmethod instead.Here's the fix:
} else if (notification.type === "discord") { - this.sendDiscordNotification(networkResponse, notification.address); + await this.sendWebhookNotification(networkResponse, notification.address, 'discord'); } else if (notification.type === "slack") { - this.sendSlackNotification(networkResponse, notification.address); + await this.sendWebhookNotification(networkResponse, notification.address, 'slack'); } else if (notification.type === "telegram") { - this.sendTelegramNotification(networkResponse, notification.address); + await this.sendWebhookNotification(networkResponse, notification.address, 'telegram'); }Likely invalid or redundant comment.
| text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` | ||
| }; | ||
| url = `https://api.telegram.org/bot${botToken}/sendMessage`; | ||
| } |
There was a problem hiding this comment.
Is this if/else ladder meant to fall through at the end?
Should we continue execution if the platform is not one of slack, discord, or telegram? Or are we meant to return early?
There was a problem hiding this comment.
No, sorry, you are right. It should not continue execution. I wrote an else to return early.
| } else if (platform === 'discord') { | ||
| message = { content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }; | ||
| } else if (platform === 'telegram') { | ||
| const [botToken, chatId] = address.split('|').map(part => part?.trim()); |
There was a problem hiding this comment.
As mentioned in previous review this should be in its own field rather than shoehorned into the address field
| chat_id: chatId, | ||
| text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` | ||
| }; | ||
| url = `https://api.telegram.org/bot${botToken}/sendMessage`; |
There was a problem hiding this comment.
This will be unnecessary once the token and chat ID are moved to their own fields
There was a problem hiding this comment.
Bot token and chat ID have been moved to their own separate fields.
| let url = address; | ||
|
|
||
| if (platform === 'slack') { | ||
| message = { text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }; |
There was a problem hiding this comment.
These can be refactored out to a template, it will clean the function up and make maintaining the messages easeir.
There was a problem hiding this comment.
Refactored out to a template for easier message maintenance.
|
|
||
| console.error(`Error sending ${platform} notification:`, error.toJSON()); | ||
| console.error(`URL: ${url}`); | ||
| console.error(`Message:`, message); |
There was a problem hiding this comment.
Errors should be logged using the Logger and standard format
| if (networkResponse.statusChanged === false) return false; | ||
|
|
||
| // if prevStatus is undefined, monitor is resuming, we're done |
There was a problem hiding this comment.
Leave these comments please, they are here to remind me what everything does and why
There was a problem hiding this comment.
Returned comments. My mistake!
| } else if (notification.type === "slack") { | ||
| this.sendSlackNotification(networkResponse, notification.address); | ||
| } else if (notification.type === "telegram") { | ||
| this.sendTelegramNotification(networkResponse, notification.address); |
There was a problem hiding this comment.
these should all be updated to use the webhook function?
| } | ||
|
|
||
| try { | ||
| await axios.post(url, message, { |
There was a problem hiding this comment.
This still needs to be moved to the NetworkService
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Server/service/notificationService.js (1)
22-59: 🛠️ Refactor suggestionRefactor webhook message construction and improve security.
Several improvements needed:
- Token and chatId should be stored in dedicated fields rather than concatenated in the address field
- Message templates can be extracted to reduce duplication
- Consider using environment variables for API base URLs
Apply this diff to extract message templates:
+const MESSAGE_TEMPLATES = { + slack: (monitor, status) => ({ + text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }), + discord: (monitor, status) => ({ + content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }), + telegram: (monitor, status) => ({ + text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }) +}; async sendWebhookNotification(networkResponse, address, platform) { const { monitor, status } = networkResponse; let message; let url = address; if (platform === 'slack') { - message = { text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }; + message = MESSAGE_TEMPLATES.slack(monitor, status); } else if (platform === 'discord') { - message = { content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }; + message = MESSAGE_TEMPLATES.discord(monitor, status); } else if (platform === 'telegram') { const [botToken, chatId] = address.split('|').map(part => part?.trim()); if (!botToken || !chatId) { return false; } message = { chat_id: chatId, - text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + ...MESSAGE_TEMPLATES.telegram(monitor, status) }; url = `https://api.telegram.org/bot${botToken}/sendMessage`; }
🧹 Nitpick comments (2)
Server/service/notificationService.js (1)
102-127: Consider using early return pattern for better readability.The function structure could be improved by returning early when notifications aren't needed.
async handleStatusNotifications(networkResponse) { try { - if (networkResponse.statusChanged === false) return false; - if (networkResponse.prevStatus === undefined) return false; + if (!networkResponse.statusChanged || networkResponse.prevStatus === undefined) { + return false; + } const notifications = await this.db.getNotificationsByMonitorId(networkResponse.monitorId);Server/service/networkService.js (1)
339-382: Consider adding rate limiting for webhook requests.While the implementation is solid, adding rate limiting would prevent potential abuse and align with platform-specific rate limits.
+const RATE_LIMITS = { + discord: { requests: 5, period: 5000 }, // 5 requests per 5 seconds + slack: { requests: 1, period: 1000 }, // 1 request per second + telegram: { requests: 30, period: 1000 } // 30 requests per second +}; async requestWebhook(platform, url, message) { try { + const limit = RATE_LIMITS[platform]; + if (limit) { + await this.rateLimiter.checkLimit(platform, limit.requests, limit.period); + } + const { response, responseTime, error } = await this.timeRequest(() =>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Server/index.js(5 hunks)Server/service/networkService.js(1 hunks)Server/service/notificationService.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
Server/service/notificationService.js (2)
14-20: LGTM! Constructor changes align with NetworkService integration.The addition of networkService parameter follows the feedback about centralizing network operations.
43-58: LGTM! Error handling looks solid.The error handling follows the established pattern:
- Detailed error logging with context
- Graceful error recovery
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/service/notificationService.js (1)
110-111: Let's keep it organized like mom's spaghetti! 🍝Extract the platform types to constants at the top of the file:
+ const NOTIFICATION_PLATFORMS = { + DISCORD: 'discord', + SLACK: 'slack', + TELEGRAM: 'telegram', + EMAIL: 'email' + }; + + const WEBHOOK_PLATFORMS = [ + NOTIFICATION_PLATFORMS.DISCORD, + NOTIFICATION_PLATFORMS.SLACK, + NOTIFICATION_PLATFORMS.TELEGRAM + ]; - } else if (["discord", "slack", "telegram"].includes(notification.type)) { + } else if (WEBHOOK_PLATFORMS.includes(notification.type)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/service/notificationService.js(2 hunks)
🔇 Additional comments (2)
Server/service/notificationService.js (2)
12-18: Yo, these constructor changes are straight fire! 🔥The addition of networkService parameter aligns perfectly with the previous feedback about centralizing network operations.
20-57: 🛠️ Refactor suggestionYo dawg, let's make this webhook code even more fire! 🎵
A few suggestions to level up this implementation:
- The message templates for each platform are nearly identical. Consider extracting them to a template:
+ const createMessage = (monitor, status, platform) => { + const text = `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`; + switch(platform) { + case 'slack': return { text }; + case 'discord': return { content: text }; + case 'telegram': return { chat_id: chatId, text }; + default: throw new Error(`Unsupported platform: ${platform}`); + } + };
- The Telegram address parsing is a bit sketchy:
- const [botToken, chatId] = address.split('|').map(part => part?.trim()); + const TELEGRAM_SEPARATOR = '|'; + const [botToken, chatId] = address.split(TELEGRAM_SEPARATOR).map(part => part?.trim()); + if (!botToken || !chatId) { + this.logger.warn({ + message: 'Invalid Telegram address format', + service: this.SERVICE_NAME, + method: 'sendWebhookNotification', + address + }); + return false; + }
- Consider adding platform-specific URL validation:
✅ Verification successful
Heads up: Notification Service Webhook Code Validated
The regex search confirms that the Telegram URL is hardcoded as
https://api.telegram.org/bot${botToken}/sendMessage, which supports the review comment’s suggestion for platform-specific URL handling. Additionally, the review suggestions for extracting message templates and improving Telegram address parsing remain applicable, as the code still embeds these implementations directly.
- The hardcoded Telegram URL in
Server/service/notificationService.jsis verified.- There’s an opportunity to add a constant separator for Telegram address parsing and log a warning on invalid formats.
- Consider refactoring message construction into a shared template for all platforms.
His palms are sweaty, knees weak, arms are heavy—let’s polish this webhook code to make it even more robust!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for hardcoded URLs in the codebase rg -i "api\.telegram\.org|api\.slack\.com|discord\.com"Length of output: 250
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Server/service/notificationService.js (2)
2-2: 🛠️ Refactor suggestionMove base URL to central configuration.
Base URLs should be maintained in a central configuration file for easier maintenance and consistency across the application.
Consider moving this constant to a central configuration file (e.g.,
config/urls.js):-const TELEGRAM_API_BASE_URL = "https://api.telegram.org/bot";
31-34: 🛠️ Refactor suggestionRefactor Telegram credentials storage.
Using address field to store both token and chat ID is a hacky solution.
Consider adding proper fields in the notification schema:
-const [botToken, chatId] = address.split('|').map(part => part?.trim()); -if (!botToken || !chatId) { - return false; -}
🧹 Nitpick comments (1)
Server/service/notificationService.js (1)
13-18: Add JSDoc for networkService parameter.The constructor's documentation is missing the networkService parameter.
/** * Creates an instance of NotificationService. * * @param {Object} emailService - The email service used for sending notifications. * @param {Object} db - The database instance for storing notification data. * @param {Object} logger - The logger instance for logging activities. + * @param {Object} networkService - The network service for making HTTP requests. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/service/notificationService.js(3 hunks)
🔇 Additional comments (3)
Server/service/notificationService.js (3)
26-40: Extract message templates to a separate configuration.Message templates should be maintained separately for easier maintenance and localization support.
Consider creating a message template configuration:
const notificationTemplates = { slack: (monitor, status) => ({ text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }), discord: (monitor, status) => ({ content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }), telegram: (monitor, status) => ({ text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` }) };
111-112: LGTM! Clean implementation of webhook notifications.The changes effectively integrate webhook notifications while maintaining consistent error handling.
21-58: Add test coverage for webhook notifications.The new webhook notification functionality should be covered by unit tests to ensure reliable operation.
Consider adding tests for:
- Different notification platforms
- Error scenarios
- Invalid Telegram credentials
- Network request failures
Would you like me to help generate the test cases?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Server/service/notificationService.js (1)
2-2: 🛠️ Refactor suggestionYo dawg, let's move this base URL to a central config!
Based on previous feedback, all base URLs should be in one place for easy maintenance.
Let's move this to a central configuration file where we keep all our API base URLs.
🧹 Nitpick comments (1)
Server/service/notificationService.js (1)
21-65: Yo, these message templates are looking repetitive!While the webhook implementation is solid, we could make it even better by extracting those message templates.
Here's how we could clean this up:
+ const NOTIFICATION_TEMPLATES = { + slack: (monitor, status) => ({ + text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }), + discord: (monitor, status) => ({ + content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }), + telegram: (monitor, status, chatId) => ({ + chat_id: chatId, + text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` + }) + }; async sendWebhookNotification(networkResponse, address, platform, botToken, chatId) { const { monitor, status } = networkResponse; - let message; let url = address; + if (!NOTIFICATION_TEMPLATES[platform]) { + this.logger.warn({ + message: `Unsupported platform: ${platform}`, + service: this.SERVICE_NAME, + method: 'sendWebhookNotification', + platform + }); + return false; + } if (platform === 'telegram') { if (!botToken || !chatId) { return false; } url = `${TELEGRAM_API_BASE_URL}${botToken}/sendMessage`; } + const message = platform === 'telegram' + ? NOTIFICATION_TEMPLATES[platform](monitor, status, chatId) + : NOTIFICATION_TEMPLATES[platform](monitor, status);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/service/notificationService.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
Server/service/notificationService.js (3)
13-19: Nice job moving network operations to NetworkService!The constructor changes look clean and align with the previous feedback about keeping network operations in one place.
124-131: Error handling looking clean, fam!The error handling with proper logging and stack trace is on point.
1-242: Mom's spaghetti... I mean, solid implementation!The changes not only implement Discord webhook notifications but also include support for Slack and Telegram in a clean, maintainable way. The code follows the feedback from previous reviews and implements proper error handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 164-164: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 165-165: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 166-168: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| for (const notification of notifications) { | ||
| if (notification.type === "email") { | ||
| this.sendEmail(networkResponse, notification.address); | ||
| } | ||
| } else if (["discord", "slack", "telegram"].includes(notification.type)) { | ||
| this.sendWebhookNotification(networkResponse, notification.address, notification.type); | ||
|
|
||
| // Handle other types of notifications here | ||
| } | ||
| } |
There was a problem hiding this comment.
Yo, there's a syntax error in this loop!
The for loop is missing a closing brace.
Here's the fix:
for (const notification of notifications) {
if (notification.type === "email") {
this.sendEmail(networkResponse, notification.address);
} else if (["discord", "slack", "telegram"].includes(notification.type)) {
this.sendWebhookNotification(networkResponse, notification.address, notification.type);
}
- // Handle other types of notifications here
}
+ }📝 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.
| for (const notification of notifications) { | |
| if (notification.type === "email") { | |
| this.sendEmail(networkResponse, notification.address); | |
| } | |
| } else if (["discord", "slack", "telegram"].includes(notification.type)) { | |
| this.sendWebhookNotification(networkResponse, notification.address, notification.type); | |
| // Handle other types of notifications here | |
| } | |
| } | |
| for (const notification of notifications) { | |
| if (notification.type === "email") { | |
| this.sendEmail(networkResponse, notification.address); | |
| } else if (["discord", "slack", "telegram"].includes(notification.type)) { | |
| this.sendWebhookNotification(networkResponse, notification.address, notification.type); | |
| } | |
| // Handle other types of notifications here | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/service/notificationService.js (1)
21-34: Yo dawg, let's make this code even more robust! 💪Consider defining platform names as constants to avoid magic strings and make maintenance easier.
+ const PLATFORMS = { + TELEGRAM: 'telegram', + SLACK: 'slack', + DISCORD: 'discord' + }; formatNotificationMessage(monitor, status, platform, chatId) { const messageText = `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`; - if (platform === 'telegram') { + if (platform === PLATFORMS.TELEGRAM) { return { chat_id: chatId, text: messageText }; } - if (platform === 'slack') { + if (platform === PLATFORMS.SLACK) { return { text: messageText }; } - if (platform === 'discord') { + if (platform === PLATFORMS.DISCORD) { return { content: messageText }; } return null; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/service/notificationService.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
Server/service/notificationService.js (4)
2-2: Yo, these changes are fire! 🔥The addition of
TELEGRAM_API_BASE_URLconstant andnetworkServiceintegration aligns perfectly with our architectural goals.Also applies to: 13-18
36-74: Mom's spaghetti... I mean, this code is ready! 🍝The webhook notification implementation is clean with proper error handling and logging. Nice job moving the network operations to
networkService!
21-74: Vomit on his sweater already... I mean, let's verify this scope! 🎯The implementation includes support for Slack and Telegram in addition to Discord. While this is awesome, we should verify if this scope expansion was intentional and documented in the PR description.
122-130:⚠️ Potential issueKnees weak, arms are heavy... we got a syntax error! 🔄
The for loop is missing its closing brace.
for (const notification of notifications) { if (notification.type === "email") { this.sendEmail(networkResponse, notification.address); } else if (["discord", "slack", "telegram"].includes(notification.type)) { this.sendWebhookNotification(networkResponse, notification.address, notification.type); } // Handle other types of notifications here - } + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Looking pretty good! I think we can replace all three types in the Notification model with just "webhook" as they are all the same type of notification.
It would make sense to have different types if they all needed to be handled differently, but all webhooks behave the same.
Other than that couple of minor changes and I think this will be ready to test, then we cna merge!
| let url = address; | ||
|
|
||
| const message = this.formatNotificationMessage(monitor, status, platform, chatId); | ||
| if (!message) { |
There was a problem hiding this comment.
Can we be explicit witht the check here?
if(message !== null)
is what we mean here right? I hate the ambiguity of truthy/fasley values 😛
| @@ -8,8 +8,17 @@ const NotificationSchema = mongoose.Schema( | |||
| }, | |||
| type: { | |||
| type: String, | |||
| enum: ["email", "sms"], | |||
| enum: ["email", "sms", "discord", "slack", "telegram"], | |||
There was a problem hiding this comment.
I don't think we need all these types anymore do we? They are all of type "webhook" now
| for (const notification of notifications) { | ||
| if (notification.type === "email") { | ||
| this.sendEmail(networkResponse, notification.address); | ||
| } | ||
| } else if (["discord", "slack", "telegram"].includes(notification.type)) { |
There was a problem hiding this comment.
This can be simplified to just if(notification.type === "webhook") as these all function the same way.
| @@ -284,6 +295,7 @@ const startApp = async () => { | |||
| app.use("/api/v1/queue", verifyJWT, queueRoutes.getRouter()); | |||
| app.use("/api/v1/distributed-uptime", distributedUptimeRoutes.getRouter()); | |||
| app.use("/api/v1/status-page", statusPageRoutes.getRouter()); | |||
| app.use("/api/v1/notifications", notificationRoutes.getRouter()); // Add this line | |||
There was a problem hiding this comment.
This can be removed now, this was used for testing correct?
Describe your changes
Add Discord webhook notification support
Issue number
#1545
Please ensure all items are checked off before requesting a review: