develop -> demo #2961
develop -> demo #2961
Conversation
fix vite.config
Add Discord embed message formatting for notifications
Add monitor name and URL formatting for hardware notifications
…fications Fix: Skip notifications during maintenance windows
feat: Support Thailand language
chore: bump deps
Allow jsonPath to be used along expectedValue and validate the response.
…implementation Addition of incident tab in creation and config page which is same.
ajhollid
requested review from
Br0wnHammer,
Owaiseimdad and
karenvicent
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![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
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces new features for incident configuration and enhanced Discord notifications but includes a critical breaking change in the maintenance window check that could cause test failures and runtime errors if not addressed.
🌟 Strengths
- Solid implementation of new incident configuration with comprehensive validation and UI integration.
- Enhanced Discord notification support with rich embeds and improved internationalization.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/src/service/infrastructure/SuperSimpleQueue/SuperSimpleQueueHelper.js | Architecture | Breaking change in method signature risks test failures and runtime errors | path:server/tests/services/jobQueue.test.js|method:isInMaintenanceWindow |
| P2 | server/src/service/infrastructure/notificationService.js | Architecture | Adds optional parameters to sendNotification, maintaining backward compatibility | method:notifyAll |
| P2 | server/src/service/infrastructure/networkService.js | Bug | Prevents runtime errors with type checks in HTTP monitoring logic | |
| P2 | client/src/Pages/Infrastructure/Create/hooks/useInfrastructureMonitorForm.jsx | Maintainability | Adds state for new incident config fields, consistent with UI updates | path:client/src/Pages/Infrastructure/Create/index.jsx |
| P2 | client/src/Validation/validation.js | Maintainability | Ensures data integrity with validation for new incident config fields | |
| P2 | server/src/service/infrastructure/notificationUtils.js | Architecture | Changes return type of buildHardwareAlerts, handled correctly in caller | method:buildHardwareAlerts |
| P2 | server/src/locales/en.json | Documentation | Supports Discord notifications with new translation keys | path:server/src/service/system/stringService.js |
🔍 Notable Themes
- Incident Configuration: Comprehensive addition of status window settings across client form, validation, and server logic, ensuring robust feature implementation.
- Notification Enhancements: Extended support for Discord and webhook notifications with improved payload handling and internationalization.
📈 Risk Diagram
This diagram illustrates the risk of the breaking change in the maintenance window check method and its potential impact on existing callers.
sequenceDiagram
participant Caller
participant SSQHelper as SuperSimpleQueueHelper
Caller->>SSQHelper: isInMaintenanceWindow(monitorId) // Old call without teamId
Note over Caller,SSQHelper: R1(P1): Breaking change - method now requires teamId parameter, risking runtime errors
SSQHelper-->>Caller: Error or unexpected behavior due to missing parameter
⚠️ **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/service/infrastructure/notificationUtils.js
The buildHardwareAlerts method now returns an array with two elements (alerts and discordContent). This change is reflected in the handleNotifications method of the notification service. This is a breaking change to the method's return type, but it is handled correctly in the only caller. Ensure no other callers exist.
Related Code:
const [alerts, discordContent] = await this.notificationUtils.buildHardwareAlerts(networkResponse);💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
| async isInMaintenanceWindow(monitorId, teamId) { | ||
| const maintenanceWindows = await this.db.maintenanceWindowModule.getMaintenanceWindowsByMonitorId({ | ||
| monitorId: monitorId.toString(), | ||
| teamId: teamId.toString(), | ||
| }); |
There was a problem hiding this comment.
P1 | Confidence: High
The method signature changed from isInMaintenanceWindow(monitorId) to isInMaintenanceWindow(monitorId, teamId). The related context shows a test file (jobQueue.test.js) that calls this method with only one argument. This is a breaking change that will cause test failures and potentially runtime errors if other callers exist. The change aligns with the new requirement to include teamId in maintenance window queries but requires updates to all call sites.
Code Suggestion:
jobQueue.isInMaintenanceWindow(monitorId, teamId);Evidence: path:server/tests/services/jobQueue.test.js, method:isInMaintenanceWindow
| sendNotification = async ({ notification, subject, content, html, discordContent = null, webhookBody = null }) => { | ||
| const { type, address } = notification; |
There was a problem hiding this comment.
P2 | Confidence: High
The sendNotification method now accepts two new optional parameters (discordContent and webhookBody). The notifyAll method has been updated to pass these parameters. This change is backward-compatible since the parameters are optional, but it extends the API contract. All callers should be reviewed to ensure they handle these new parameters appropriately.
| const useInfrastructureMonitorForm = () => { | ||
| const [infrastructureMonitor, setInfrastructureMonitor] = useState({ | ||
| url: "", | ||
| name: "", | ||
| notifications: [], | ||
| notify_email: false, | ||
| interval: 0.25, | ||
| statusWindowSize: 5, | ||
| statusWindowThreshold: 60, |
There was a problem hiding this comment.
P2 | Confidence: High
The hook now includes new state fields (statusWindowSize and statusWindowThreshold). The related context shows that the hook is used in index.jsx, which has been updated to include form inputs for these fields. This change is consistent and well-contained. However, ensure that the server-side infrastructure monitor model supports these new fields.
client/src/Validation/validation.js
Outdated
There was a problem hiding this comment.
P2 | Confidence: High
Added validation for the new statusWindowSize and statusWindowThreshold fields. The validation rules are appropriate (1-20 for window size, 1-100 for threshold percentage). This ensures data integrity for the new incident configuration feature.
| "errorForValidEmailAddress": "A valid recipient email address is required.", | ||
| "testEmailSubject": "Test E-mail from Checkmate" | ||
| "testEmailSubject": "Test E-mail from Checkmate", | ||
| "discordNotification": { |
There was a problem hiding this comment.
P2 | Confidence: High
Added new translation keys for Discord notifications. The stringService has been updated to use these keys. This supports the new Discord embed feature and improves internationalization.
| if (expectedValue && !jsonPath) { | ||
| let ok = false; | ||
| if (matchMethod === "equal") ok = payload === expectedValue; | ||
| if (matchMethod === "include") ok = payload.includes(expectedValue); | ||
| if (matchMethod === "regex") ok = new RegExp(expectedValue).test(payload); | ||
| if (matchMethod === "include" && typeof payload === "string") ok = payload.includes(expectedValue); | ||
| if (matchMethod === "regex" && typeof payload === "string") ok = new RegExp(expectedValue).test(payload); |
There was a problem hiding this comment.
P2 | Confidence: Medium
The code now includes type checks for payload when using include and regex match methods. This prevents runtime errors if the payload is not a string. However, the change does not handle cases where expectedValue is provided with jsonPath (the condition checks !jsonPath). The related logic for JSONPath extraction appears correct but should be tested with various payload types.
No description provided.