Implement edit functionality for infrastructure monitors #1933
Conversation
WalkthroughThis pull request updates several files to enhance the handling of infrastructure monitors. The modifications include restructuring parameters and payloads for update operations, differentiating between create and edit modes, extending validation rules, and enhancing localization. In addition, a new route for configuring monitors is added, and the admin state is now dynamically determined on the details page. The changes are spread across slice functions, UI components, route definitions, network service logic, validation schema, and locale files. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as Browser
participant P as Create/Edit Page
participant S as Monitor Slice
participant N as NetworkService
participant SV as Server
U->>B: Navigate to /infrastructure/configure/:monitorId
B->>P: Render InfrastructureCreate Component
P->>P: Retrieve monitorId with useParams
alt Edit Mode (monitorId exists)
P->>P: Load existing monitor data and prefill form (including notify_email)
else Create Mode
P->>P: Display empty form for new monitor
end
U->>P: Submit form data
alt Edit Mode
P->>S: Dispatch updateInfrastructureMonitor({ monitorId, monitor })
else Create Mode
P->>S: Dispatch createInfrastructureMonitor({ monitor data })
end
S->>N: Call updateMonitor(payload) if update
N->>SV: Send PUT request with payload
SV-->>N: Return update confirmation
N-->>S: Pass response back
S-->>P: Deliver result to UI
P->>U: Display success or error message
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 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: 1
🧹 Nitpick comments (4)
src/locales/gb.json (1)
159-159: Save button text looks ready to go.This localization key provides appropriate text for the save button in edit mode. One thought - have we considered a different text for saving edits vs. creating new monitors to better indicate the action?
src/Validation/validation.js (1)
352-357: Robust validation for email notifications - nice!The validation looks well-structured, ensuring that:
- Notifications are an array of objects
- Each notification object has a required
typefield limited to "email"- Each notification has a properly formatted email address
However, I'm wondering if we should also validate the maximum number of notifications to prevent potential abuse or performance issues.
Consider adding a maximum limit to the number of notifications:
notifications: joi.array().items( joi.object({ type: joi.string().valid("email").required(), address: joi.string().email({ tlds: { allow: false } }).required(), }) - ) + ).max(10) // Limit to a reasonable numbersrc/Pages/Infrastructure/Create/index.jsx (2)
64-64: Unused variables from hookThe destructured variables
isLoadingandnetworkErrorfromuseHardwareMonitorsFetchare never used in the component, which could lead to confusion about the component's behavior.Consider one of these approaches:
- Add loading state handling to improve UX when fetching monitor data
- Remove the unused variables if they're not needed
- const { monitor, isLoading, networkError } = isCreate ? {} : useHardwareMonitorsFetch({ monitorId }); + const { monitor } = isCreate ? {} : useHardwareMonitorsFetch({ monitorId });🧰 Tools
🪛 ESLint
[error] 64-64: 'isLoading' is assigned a value but never used.
(no-unused-vars)
[error] 64-64: 'networkError' is assigned a value but never used.
(no-unused-vars)
[error] 64-64: React Hook "useHardwareMonitorsFetch" is called conditionally. React Hooks must be called in the exact same order in every component render.
(react-hooks/rules-of-hooks)
455-456: Consider using consistent loading stateThe button shows loading state based on
monitorState?.isLoadingbut doesn't account for theisLoadingfrom the hook, potentially leading to inconsistent UI feedback.Consider combining both loading states to provide consistent feedback:
- loading={monitorState?.isLoading} + loading={monitorState?.isLoading || (!isCreate && hookResult?.isLoading)}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Features/InfrastructureMonitors/infrastructureMonitorsSlice.js(1 hunks)src/Pages/Infrastructure/Create/index.jsx(13 hunks)src/Pages/Infrastructure/Details/index.jsx(2 hunks)src/Routes/index.jsx(1 hunks)src/Utils/NetworkService.js(1 hunks)src/Validation/validation.js(1 hunks)src/locales/gb.json(1 hunks)
🧰 Additional context used
🪛 ESLint
src/Pages/Infrastructure/Create/index.jsx
[error] 64-64: 'isLoading' is assigned a value but never used.
(no-unused-vars)
[error] 64-64: 'networkError' is assigned a value but never used.
(no-unused-vars)
[error] 64-64: React Hook "useHardwareMonitorsFetch" is called conditionally. React Hooks must be called in the exact same order in every component render.
(react-hooks/rules-of-hooks)
🔇 Additional comments (16)
src/Pages/Infrastructure/Details/index.jsx (2)
34-34: Great addition! The isAdmin hook implementation looks good.Using the
useIsAdmin()hook to dynamically determine admin status is a solid improvement over hardcoding the value. This change aligns well with the edit functionality being implemented in this PR.
78-78: This dynamic admin check is just what we needed.Replacing the hardcoded
falsewith the dynamicisAdminvariable allows theMonitorStatusHeadercomponent to properly reflect the user's admin status, which will control whether the edit functionality is accessible based on permissions.src/locales/gb.json (1)
153-153: Good localization key addition for edit mode.The new key for edit mode interface text looks good. This supports the edit functionality being introduced in the PR.
src/Routes/index.jsx (1)
151-154: Nice route configuration for edit functionality.The new route properly leverages the existing
InfrastructureCreatecomponent with a dynamicmonitorIdparameter, maintaining DRY principles. This pattern is consistent with the routes for other monitor types in the application.src/Features/InfrastructureMonitors/infrastructureMonitorsSlice.js (3)
97-99: Function parameter structure enhancement is well implementedThe parameter structure change from
(data, thunkApi)to({ monitorId, monitor }, thunkApi)improves code readability by making it clear what data is expected. This change aligns well with the edit functionality requirement.
106-107: Good addition of thresholds and secret propertiesReplacing
thresholdwiththresholds(plural) and adding thesecretproperty ensures the update operation includes all necessary fields. This change properly supports the monitor editing functionality.
109-113: Clean refactoring of networkService callThe updated call to
networkService.updateMonitornow correctly passes the parameters in the expected format, making the code more maintainable and consistent with the updated service method.src/Utils/NetworkService.js (1)
260-267: Flexible payload handling implementation looks greatThe changes to the
updateMonitormethod provide better flexibility by:
- Properly destructuring the parameters
- Creating a fallback payload when
updatedFieldsisn't provided- Using the payload in the PUT request
This approach maintains backward compatibility while supporting the new edit functionality for infrastructure monitors.
src/Pages/Infrastructure/Create/index.jsx (8)
73-73: Good addition of notify_email stateAdding the
notify_emailstate allows for cleaner handling of email notifications preferences, improving the user experience when editing monitors.
87-110: Well-implemented form population for edit modeThe useEffect hook correctly populates the form fields with existing monitor data when in edit mode, including proper conversions for threshold values from decimal to percentage representation. The code also handles potential missing data gracefully.
116-122: Improved notification handling logicThe simplified notification handling code makes the component more maintainable while preserving functionality. The approach to conditionally add the user's email notification is clean and effective.
196-207: Clean implementation of create/update logicThe conditional dispatch based on
isCreateelegantly handles both creation and update scenarios, with appropriate success messages. This approach minimizes code duplication while providing clear feedback to users.
236-253: Enhanced notification handling in handleNotifications functionThe updated function correctly manages the notification list and sets the
notify_emailstate based on the checkbox status. This provides a better user experience when editing notification preferences.
258-266: Well-structured breadcrumbs for different modesThe breadcrumb paths are correctly adjusted based on whether the component is in create or edit mode, providing appropriate navigation context to the user.
322-345: Good UX consideration for disabling URL in edit modeDisabling the URL input field and hiding the protocol selection when in edit mode prevents users from accidentally changing the server address, which could lead to connectivity issues. This is a thoughtful UX decision.
411-411: Ensure consistent input value typesConverting the field value to a string ensures the component handles all input values consistently, preventing potential type-related rendering issues.
![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 Core Changes
- Primary purpose and scope: Implement the ability to edit settings for existing infrastructure monitors.
- Key components modified:
infrastructureMonitorsSlice.js,Create/index.jsx,Details/index.jsx,Routes/index.jsx,NetworkService.js,validation.js,gb.json. - Cross-component impacts: Changes affect Redux state management, routing, API calls, validation, and UI components related to infrastructure monitor creation and details.
- Business value alignment: Improves user experience by allowing in-place modification of monitor configurations, reducing the need to delete and recreate monitors. Aligns with the business need for flexible and dynamic monitoring capabilities.
1.2 Technical Architecture
- System design modifications: The
Create/index.jsxcomponent now handles both creation and editing of infrastructure monitors, based on the presence of amonitorIdroute parameter. - Component interaction changes: The
Createcomponent fetches existing monitor data when in edit mode and populates the form fields. TheupdateInfrastructureMonitoraction in the Redux slice is used for saving changes. - Integration points impact: The
updateMonitorfunction inNetworkService.jsis modified to handle the update request. - Dependency changes and implications: No new dependencies were introduced.
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Incorrect Threshold Value Conversion
- Impact: Thresholds are sent as percentages (e.g., 80) instead of decimals (e.g., 0.8), causing alerts to be 100 times stricter than intended. This could lead to a flood of false-positive alerts.
- Resolution: Divide the threshold values by 100 before sending them to the backend.
Issue: Incorrect Interval Calculation
- Impact: The monitor interval is calculated incorrectly, resulting in a much shorter interval than intended (e.g., 15 seconds instead of 15 minutes). This can overload the monitored server and the monitoring system itself.
- Resolution: Use
MS_PER_HOUR = 60 * MS_PER_MINUTE(3,600,000) for interval calculations.
Issue: Authorization Bypass in Edit Mode
- Impact: Non-admin users can access the edit functionality by directly navigating to the edit URL, bypassing intended access controls. This could allow unauthorized modification of monitor configurations.
- Resolution: Implement an authorization check in
Create/index.jsxusinguseIsAdmin()and redirect unauthorized users.
2.2 Should Fix (P1🟡)
Issue: Notification Data Handling
- Impact: Legacy string-format notifications are lost during edits, potentially causing users to miss critical alerts.
- Suggested Solution: Update the notification handling logic to correctly handle both object and string formats during the edit population.
Issue: Email Notification State
- Impact: The
notify_emailcheckbox state is incorrect when non-email alerts exist, leading to a confusing user experience. - Suggested Solution: Update the logic to check for email notifications specifically for the current user's email.
2.3 Consider (P2🟢)
Area: General Improvements
- Improvement Opportunity: Enhance user experience and application robustness. Suggestions include adding loading states, implementing form dirty state tracking, adding a confirmation modal for unsaved changes, enhancing validation error messages, and adding audit logging for configuration changes.
2.4 Summary of Action Items
- Immediate (P0): Fix threshold conversion, interval calculation, and authorization bypass.
- Before Merge (P1): Correct notification data handling and email notification state.
- Future Iterations (P2): Consider general improvements for enhanced usability and robustness.
3. Technical Analysis
3.1 Code Logic Analysis
📁 src/Pages/Infrastructure/Create/index.jsx - CreateInfrastructureMonitor
- Submitted PR Code:
// ... (rest of the component)
const handleCreateInfrastructureMonitor = async (event) => {
event.preventDefault();
// ... (other form building logic)
let form = {
// ... (other fields)
thresholds,
};
// ... (validation and dispatch logic)
};- Analysis:
- Current logic: The
handleCreateInfrastructureMonitorfunction builds the form data and dispatches eithercreateInfrastructureMonitororupdateInfrastructureMonitorbased on whethermonitorIdis present. The threshold values are directly taken from the component's state. - Potential issues: The threshold values are not correctly converted to decimals before being sent to the backend.
- Edge cases and error handling: The code handles validation errors and displays them to the user.
- Cross-component impact: Affects the
infrastructureMonitorsSlice.jsandNetworkService.js. - Business logic considerations: Incorrect threshold values will lead to incorrect alert behavior.
- Current logic: The
- LlamaPReview Suggested Improvements:
const handleCreateInfrastructureMonitor = async (event) => {
event.preventDefault();
const formattedNotifications = infrastructureMonitor.notifications.map((n) =>
typeof n === "string" ? { type: "email", address: n } : n
);
if (infrastructureMonitor.notify_email) {
formattedNotifications.push({ type: "email", address: user.email });
}
// Build the form
let form = {
url: `http${https ? "s" : ""}://` + infrastructureMonitor.url,
name:
infrastructureMonitor.name === ""
? infrastructureMonitor.url
: infrastructureMonitor.name,
interval: infrastructureMonitor.interval * MS_PER_HOUR, // Corrected interval calculation
cpu: infrastructureMonitor.cpu,
...(infrastructureMonitor.cpu
? { usage_cpu: parseFloat(infrastructureMonitor.usage_cpu) / 100 } // Corrected threshold conversion
: {}),
memory: infrastructureMonitor.memory,
...(infrastructureMonitor.memory
? { usage_memory: parseFloat(infrastructureMonitor.usage_memory) / 100 } // Corrected threshold conversion
: {}),
disk: infrastructureMonitor.disk,
...(infrastructureMonitor.disk
? { usage_disk: parseFloat(infrastructureMonitor.usage_disk) / 100 } // Corrected threshold conversion
: {}),
temperature: infrastructureMonitor.temperature,
...(infrastructureMonitor.temperature
? { usage_temperature: parseFloat(infrastructureMonitor.usage_temperature) / 100 } // Corrected threshold conversion
: {}),
secret: infrastructureMonitor.secret,
notifications: formattedNotifications,
};
// ... (rest of the function)
// Add authorization check for edit mode
useEffect(() => {
if (!isCreate && !isAdmin) {
navigate("/infrastructure");
createToast({ body: t("unauthorizedAccess") });
}
}, [isAdmin, isCreate, navigate, t]);
};
// Define MS_PER_HOUR
const MS_PER_MINUTE = 60 * 1000;
const MS_PER_HOUR = 60 * MS_PER_MINUTE;- Improvement rationale:
- Technical benefits: Corrects the threshold and interval values, ensuring accurate monitor behavior. Adds authorization check to prevent unauthorized access.
- Business value: Prevents incorrect alerts and unauthorized modifications, improving system reliability and security.
- Risk assessment: Low risk; these changes are essential for correct functionality.
📁 src/Pages/Infrastructure/Create/index.jsx - useEffect for populating form fields
- Submitted PR Code:
useEffect(() => {
if (isCreate || !monitor) return;
setInfrastructureMonitor({
url: monitor.url.replace(/^https?:\/\//, ""),
name: monitor.name || "",
notifications: monitor.notifications?.filter(n => typeof n === "object") || [],
notify_email: (monitor.notifications?.length ?? 0) > 0,
interval: monitor.interval / MS_PER_MINUTE,
cpu: monitor.thresholds?.usage_cpu !== undefined,
usage_cpu: monitor.thresholds?.usage_cpu ? monitor.thresholds.usage_cpu * 100 : "",
memory: monitor.thresholds?.usage_memory !== undefined,
usage_memory: monitor.thresholds?.usage_memory ? monitor.thresholds.usage_memory * 100 : "",
disk: monitor.thresholds?.usage_disk !== undefined,
usage_disk: monitor.thresholds?.usage_disk ? monitor.thresholds.usage_disk * 100 : "",
temperature: monitor.thresholds?.usage_temperature !== undefined,
usage_temperature: monitor.thresholds?.usage_temperature ? monitor.thresholds.usage_temperature * 100 : "",
secret: monitor.secret || "",
});
setHttps(monitor.url.startsWith("https"));
}, [isCreate, monitor]);- Analysis:
- Current logic: Populates the form fields with data from the fetched monitor object when in edit mode.
- Potential issues: Incorrectly handles legacy string-format notifications and the
notify_emailstate. UsesMS_PER_MINUTEinstead ofMS_PER_HOUR. - Edge cases and error handling: Handles cases where
monitoror its properties are undefined. - Cross-component impact: Affects the display and editing of monitor data.
- Business logic considerations: Incorrect notification handling can lead to missed alerts.
- LlamaPReview Suggested Improvements:
useEffect(() => {
if (isCreate || !monitor) return;
setInfrastructureMonitor({
url: monitor.url.replace(/^https?:\/\//, ""),
name: monitor.name || "",
notifications: monitor.notifications?.map(n =>
typeof n === 'string' ? { type: 'email', address: n } : n
) || [], // Corrected notification handling
notify_email: monitor.notifications?.some(n =>
n.type === 'email' && n.address === user.email
) ?? false, // Corrected notify_email logic
interval: monitor.interval / MS_PER_HOUR, // Corrected interval calculation
cpu: monitor.thresholds?.usage_cpu !== undefined,
usage_cpu: monitor.thresholds?.usage_cpu ? monitor.thresholds.usage_cpu * 100 : "",
memory: monitor.thresholds?.usage_memory !== undefined,
usage_memory: monitor.thresholds?.usage_memory ? monitor.thresholds.usage_memory * 100 : "",
disk: monitor.thresholds?.usage_disk !== undefined,
usage_disk: monitor.thresholds?.usage_disk ? monitor.thresholds.usage_disk * 100 : "",
temperature: monitor.thresholds?.usage_temperature !== undefined,
usage_temperature: monitor.thresholds?.usage_temperature ? monitor.thresholds.usage_temperature * 100 : "",
secret: monitor.secret || "",
});
setHttps(monitor.url.startsWith("https"));
}, [isCreate, monitor, user.email]); // Added user.email to dependency array- Improvement rationale:
- Technical benefits: Correctly handles all notification formats and the
notify_emailstate. Uses the correct interval calculation. - Business value: Ensures accurate data representation and prevents missed alerts.
- Risk assessment: Low risk; these changes improve data consistency and accuracy.
- Technical benefits: Correctly handles all notification formats and the
📁 src/Utils/NetworkService.js - updateMonitor
- Submitted PR Code:
async updateMonitor(config) {
const { monitorId, monitor, updatedFields } = config;
const payload = updatedFields ?? {
name: monitor.name,
description: monitor.description,
interval: monitor.interval,
notifications: monitor.notifications,
};
return this.axiosInstance.put(`/monitors/${monitorId}`, payload, {
headers: {
"Content-Type": "application/json",
},
});
}-
Analysis:
- Current Logic: The function constructs a payload using either provided
updatedFieldsor a default object derived from themonitorobject. - Potential Issues: The logic for constructing the default payload is redundant and can be simplified. It's better to rely solely on the
updatedFieldspassed to the function. - Cross-component impact: This function is called by the
updateInfrastructureMonitorthunk ininfrastructureMonitorsSlice.js. - Business Logic Considerations: Unnecessary complexity in constructing the payload.
- Current Logic: The function constructs a payload using either provided
-
LlamaPReview Suggested Improvements:
async updateMonitor({ monitorId, updatedFields }) {
return this.axiosInstance.put(`/monitors/${monitorId}`, updatedFields, {
headers: {
"Content-Type": "application/json",
},
});
}- Improvement rationale:
- Technical benefits: Simplifies the function and removes redundant logic. Improves code readability and maintainability.
- Business value: No direct business value, but improves code quality.
- Risk assessment: Low risk; this change simplifies the function without altering its core functionality.
3.2 Key Quality Aspects
- System scalability considerations: The changes do not introduce any significant scalability concerns.
- Performance bottlenecks and optimizations: The corrected interval calculation prevents excessive monitoring requests, improving performance.
- Testing strategy and coverage: The PR description mentions self-review and testing. However, adding specific integration tests for the edit flow, threshold conversions, authorization, and notification handling is recommended.
- Documentation needs: No significant documentation changes are required.
4. Overall Evaluation
- Technical assessment: The PR introduces valuable functionality for editing infrastructure monitors. However, it contains critical issues related to data handling and authorization that must be addressed.
- Business impact: The PR enhances the user experience and operational efficiency by allowing in-place monitor configuration updates.
- Risk evaluation: The identified critical issues pose a moderate risk to system functionality and security.
- Notable positive aspects and good practices: The PR effectively reuses existing UI components and maintains a consistent validation approach.
- Implementation quality: The implementation is generally good, but requires fixes for the identified issues.
- Final recommendation: Request Changes. The critical issues (P0) must be addressed before merging. The P1 issues should also be addressed before merging.
💡 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.
|
Thanks @vishnusn77 - UI wise looks good to me! |
|
Yep looks good visually! @vishnusn77 I'll review as soon as possible, it's a bigger PR so might take me a little bit, thanks for your patience! |
| const isCreate = typeof monitorId === "undefined"; | ||
|
|
||
| // Fetch monitor details if editing | ||
| const { monitor, isLoading, networkError } = isCreate ? {} : useHardwareMonitorsFetch({ monitorId }); |
There was a problem hiding this comment.
React doesn't always work as expected if hooks are conditionally called. Better to add a check to the userHardwareFetch hook to abort early if isCreate === true
This should throw an error in your linter, make sure you've got it on and set up for React
| url: monitor.url.replace(/^https?:\/\//, ""), | ||
| name: monitor.name || "", | ||
| notifications: monitor.notifications?.filter(n => typeof n === "object") || [], | ||
| notify_email: (monitor.notifications?.length ?? 0) > 0, |
There was a problem hiding this comment.
There will be other types of notifictions besides email in the near future, we need check this only if the notiication type is "email"
Describe your changes
Issue number
#1277
Mention the issue number(s) this PR addresses (e.g., #123).
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):Screen.Recording.2025-03-15.030658.mp4