[Netmanager] Added maintenance banner, fixed sidebar, use skip for pagination #2300
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements related to maintenance status management within the application. A new constant for fetching maintenance status is defined, alongside a custom React hook that manages and polls maintenance status. Additionally, a new Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
netmanager/src/utils/hooks/useMaintenanceStatus.js (1)
9-25: Consider enhancing error handling and polling configuration.While the implementation is solid, consider these improvements:
- Make the polling interval configurable via props
- Add a retry mechanism for failed requests
- Consider using exponential backoff for retries
Here's a suggested enhancement:
-export const useMaintenanceStatus = () => { +export const useMaintenanceStatus = ({ pollingInterval = 5 * 60 * 1000, maxRetries = 3 } = {}) => { const [maintenance, setMaintenance] = useState(null); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); + const [retryCount, setRetryCount] = useState(0); useEffect(() => { const checkMaintenanceStatus = async () => { try { const response = await getMaintenanceStatusApi(); setMaintenance(response.maintenance); + setRetryCount(0); } catch (err) { + if (retryCount < maxRetries) { + setRetryCount(prev => prev + 1); + // Exponential backoff + setTimeout(checkMaintenanceStatus, Math.pow(2, retryCount) * 1000); + } setError(err); } finally { setLoading(false); } }; checkMaintenanceStatus(); - const interval = setInterval(checkMaintenanceStatus, 5 * 60 * 1000); + const interval = setInterval(checkMaintenanceStatus, pollingInterval); return () => clearInterval(interval); - }, []); + }, [pollingInterval, maxRetries, retryCount]);netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (2)
5-5: Consider using absolute imports.The relative import path could be simplified using absolute imports.
-import { formatDateString } from '../../../utils/dateTime'; +import { formatDateString } from 'utils/dateTime';
29-36: Add accessibility attributes to the Alert component.Consider adding ARIA attributes to improve accessibility.
return ( <Alert severity="warning" className={classes.banner} + role="alert" + aria-live="polite" > {maintenance.message || 'System maintenance in progress. Some features may be unavailable.'} Estimated downtime: {formatDateString(maintenance.startDate)} -{' '} {formatDateString(maintenance.endDate)} </Alert> );netmanager/src/views/apis/authService.js (1)
114-118: Consider adding error handling documentationThe implementation follows the established pattern and looks good. However, since this is a new API endpoint that will be used for displaying maintenance status to users, it would be helpful to document the expected error scenarios and how they should be handled by consumers.
+/** + * Fetches the current maintenance status + * @returns {Promise<Object>} The maintenance status data + * @throws {AxiosError} When the request fails + */ export const getMaintenanceStatusApi = async () => { return await createAxiosInstance() .get(GET_MAINTENANCE_STATUS) .then((response) => response.data); };netmanager/src/views/layouts/Main.js (1)
168-168: Consider adding error boundaries and layout adjustmentsWhile the placement of MaintenanceBanner is logical, consider these improvements:
- Wrap the component in an error boundary to prevent the entire layout from breaking if the maintenance status fetch fails
- Adjust the root paddingTop calculation to account for the banner height when visible
Example implementation:
+ import { ErrorBoundary } from 'react-error-boundary'; const useStyles = makeStyles((theme) => ({ root: { - paddingTop: 56, + paddingTop: ({ hasMaintenanceBanner }) => hasMaintenanceBanner ? 96 : 56, height: '100%', [theme.breakpoints.up('sm')]: { - paddingTop: 64 + paddingTop: ({ hasMaintenanceBanner }) => hasMaintenanceBanner ? 104 : 64 } }, // ... other styles })); const Main = (props) => { const { children } = props; + const [hasMaintenanceBanner, setHasMaintenanceBanner] = useState(false); - const classes = useStyles(); + const classes = useStyles({ hasMaintenanceBanner }); // ... other code return ( <div className={clsx({ [classes.root]: true, [classes.shiftContent]: isDesktop })}> - <MaintenanceBanner /> + <ErrorBoundary fallback={null}> + <MaintenanceBanner onStatusChange={setHasMaintenanceBanner} /> + </ErrorBoundary> <Topbar toggleSidebar={toggleSidebar} /> // ... rest of the JSX
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
netmanager/src/config/urls/authService.js(1 hunks)netmanager/src/utils/hooks/useMaintenanceStatus.js(1 hunks)netmanager/src/views/apis/authService.js(2 hunks)netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js(1 hunks)netmanager/src/views/layouts/Main.js(2 hunks)netmanager/src/views/layouts/common/Sidebar/Sidebar.js(0 hunks)netmanager/src/views/pages/UserList/AvailableUserList.js(3 hunks)netmanager/src/views/pages/UserList/UserList.js(4 hunks)
💤 Files with no reviewable changes (1)
- netmanager/src/views/layouts/common/Sidebar/Sidebar.js
🔇 Additional comments (8)
netmanager/src/config/urls/authService.js (1)
32-33: LGTM! The endpoint follows RESTful conventions.
The new maintenance status endpoint is well-structured and consistent with existing patterns.
netmanager/src/utils/hooks/useMaintenanceStatus.js (1)
4-8: LGTM! Well-structured state management.
The hook properly initializes and manages separate states for maintenance status, loading state, and errors.
netmanager/src/views/apis/authService.js (1)
8-9: LGTM! Clean import addition
The GET_MAINTENANCE_STATUS import follows the established pattern of importing URL constants.
netmanager/src/views/layouts/Main.js (2)
22-22: LGTM! Clean import addition
The MaintenanceBanner import is appropriately placed with other component imports.
168-168: Verify the banner's impact on existing components
The MaintenanceBanner might affect the positioning of other components in the layout. Let's verify its integration with existing components.
✅ Verification successful
MaintenanceBanner implementation is safe for layout integration
The MaintenanceBanner component uses fixed positioning with appropriate z-index (9999) to ensure it stays on top without disrupting other components. The banner only renders when maintenance is active and positions itself at the very top of the viewport, which is a standard practice for system-wide notifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might be affected by the banner's presence
# Look for components using position: fixed/absolute or z-index
rg -g '*.{js,jsx,css}' -A 2 'position:\s*(fixed|absolute)|z-index' src/views/
Length of output: 130
Script:
#!/bin/bash
# Let's first find the correct path to the views directory
fd -t d "views"
# Then search for MaintenanceBanner component definition
fd -e js -e jsx "MaintenanceBanner"
# And look for style-related patterns in those files
rg -g '*.{js,jsx,css}' 'position:|z-index:|style='
Length of output: 67394
Script:
#!/bin/bash
# Let's check the MaintenanceBanner component implementation
cat netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js
Length of output: 1123
netmanager/src/views/pages/UserList/AvailableUserList.js (3)
28-29: LGTM! Good choice on pagination variable names.
The change from pageSize/currentPage to limit/skip better reflects the underlying API parameters and makes the offset-based pagination more explicit.
32-38: LGTM! Clean implementation of the pagination parameters.
The function signature and API parameters are well-aligned with the new pagination strategy while maintaining robust error handling.
67-71: Consider handling pagination edge cases.
While the implementation works, there are two potential issues to consider:
- The skip calculation (
page * pageSize) could overflow for very large page numbers - The
currentPagecalculation (skip / limit) might result in floating-point precision issues
Let's verify the maximum possible values:
Consider adding bounds checking:
const handlePageChange = (page, pageSize) => {
+ if (page < 0 || pageSize <= 0) return;
const newSkip = page * pageSize;
+ if (newSkip >= Number.MAX_SAFE_INTEGER) return;
setSkip(newSkip);
setLimit(pageSize);
fetchUsers(newSkip, pageSize);
};Also applies to: 82-83
| {maintenance.message || | ||
| 'System maintenance in progress. SoformatDate(me features may be unavailable.'} |
There was a problem hiding this comment.
Fix typo in default maintenance message.
There appears to be a typo in the default message: "SoformatDate(me" should likely be "Some".
- {maintenance.message ||
- 'System maintenance in progress. SoformatDate(me features may be unavailable.'}
+ {maintenance.message ||
+ 'System maintenance in progress. Some features may be unavailable.'}📝 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.
| {maintenance.message || | |
| 'System maintenance in progress. SoformatDate(me features may be unavailable.'} | |
| {maintenance.message || | |
| 'System maintenance in progress. Some features may be unavailable.'} |
| const [limit, setLimit] = useState(10); | ||
| const [skip, setSkip] = useState(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting shared pagination logic.
The pagination implementation is duplicated between UserList.js and AvailableUserList.js. This includes state management, fetch logic, and page calculation.
Consider creating a custom hook to share this logic:
// usePagination.js
const usePagination = (fetchFn, initialLimit = 10) => {
const [limit, setLimit] = useState(initialLimit);
const [skip, setSkip] = useState(0);
const handlePageChange = (page, pageSize) => {
if (page < 0 || pageSize <= 0) return;
const newSkip = page * pageSize;
if (newSkip >= Number.MAX_SAFE_INTEGER) return;
setSkip(newSkip);
setLimit(pageSize);
fetchFn(newSkip, pageSize);
};
return {
limit,
skip,
currentPage: skip / limit,
handlePageChange
};
};This would reduce code duplication and centralize the edge case handling.
Also applies to: 41-47, 77-81, 94-95
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
1 similar comment
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (2)
7-19: Consider using a constant for the z-index value.The z-index value of 9999 is a magic number. Consider defining it as a constant to improve maintainability.
+const MAINTENANCE_BANNER_Z_INDEX = 9999; const useStyles = makeStyles((theme) => ({ banner: { position: 'fixed', top: 0, left: 0, right: 0, - zIndex: 9999, + zIndex: MAINTENANCE_BANNER_Z_INDEX, '& .MuiAlert-message': { width: '100%', textAlign: 'center' } } }));
21-36: Consider adding error boundary and accessibility improvements.The component looks good but could benefit from some enhancements:
- Wrap the Alert component with ErrorBoundary
- Add aria-live attribute for accessibility
return ( + <ErrorBoundary> - <Alert severity="warning" className={classes.banner}> + <Alert severity="warning" className={classes.banner} aria-live="polite"> {maintenance.message || 'System maintenance in progress. Some features may be unavailable.'} Estimated downtime: {formatDateString(maintenance.startDate)} -{' '} {formatDateString(maintenance.endDate)} </Alert> + </ErrorBoundary> );netmanager/src/redux/AccessControl/reducers.js (1)
41-47: Consistent reducer implementation for user dataThe reducer cases handle the new data structure appropriately, maintaining consistency between network and available users. However, consider adding data validation to ensure
action.payloadcontains the expected properties.Consider adding validation:
case LOAD_NETWORK_USERS_SUCCESS: + const { users, total } = action.payload || {}; return { ...state, networkUsers: { - users: action.payload.users, - total: action.payload.total + users: users || null, + total: total || 0 } };Also applies to: 51-57
netmanager/src/redux/AccessControl/operations.js (1)
80-97: Enhanced error handling and pagination supportThe function now properly supports pagination through params and includes improved error handling. However, consider adding type validation for the response data.
Consider adding response validation:
export const fetchNetworkUsers = (networkId, params) => async (dispatch) => { try { const resData = await getNetworkUsersListApi(networkId, params); + if (!resData || typeof resData.total_assigned_users !== 'number') { + throw new Error('Invalid response format'); + } dispatch({ type: LOAD_NETWORK_USERS_SUCCESS, payload: { users: resData.assigned_users, total: resData.total_assigned_users } }); return resData; } catch (err) { dispatch({ type: LOAD_NETWORK_USERS_FAILURE, - payload: err + payload: err.message || 'Failed to fetch network users' }); throw err; } };netmanager/src/views/apis/accessControl.js (1)
84-88: Clean API implementation with pagination supportThe implementation correctly handles optional pagination parameters with a default empty object. Consider documenting the expected params structure in JSDoc.
Add documentation:
+/** + * Fetches available users for a network with optional pagination + * @param {string} networkId - The network ID + * @param {Object} params - Optional pagination parameters (e.g., limit, skip) + * @returns {Promise<Object>} The response containing available users and total count + */ export const getAvailableNetworkUsersListApi = async (networkId, params = {}) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
netmanager/src/redux/AccessControl/operations.js(1 hunks)netmanager/src/redux/AccessControl/reducers.js(2 hunks)netmanager/src/views/apis/accessControl.js(2 hunks)netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js(1 hunks)netmanager/src/views/pages/ExportData/index.js(1 hunks)
🔇 Additional comments (4)
netmanager/src/views/pages/ExportData/index.js (1)
533-533: LGTM! Property name change aligns with multi-select functionality.
The change from 'device' to 'devices' in the request body better reflects that multiple devices can be selected.
netmanager/src/redux/AccessControl/reducers.js (1)
18-21: Well-structured state organization for pagination
The new state structure cleanly separates user data from total counts, which is ideal for pagination implementation. The consistent structure between networkUsers and availableUsers promotes maintainability.
Also applies to: 23-26
netmanager/src/redux/AccessControl/operations.js (1)
100-117: Consistent implementation for available users
The implementation mirrors fetchNetworkUsers, maintaining consistency. Consider applying the same validation improvements suggested above.
netmanager/src/views/apis/accessControl.js (1)
114-118: Consistent API implementation for network users
The implementation maintains consistency with getAvailableNetworkUsersListApi. Consider adding similar JSDoc documentation here as well.
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
netmanager/src/utils/hooks/useMaintenanceStatus.js (1)
30-30: Consider making the polling interval configurableThe 5-minute polling interval is hardcoded. Consider making it configurable through environment variables or props for flexibility.
+ const POLL_INTERVAL = process.env.REACT_APP_MAINTENANCE_POLL_INTERVAL || 5 * 60 * 1000; - const interval = setInterval(checkMaintenanceStatus, 5 * 60 * 1000); + const interval = setInterval(checkMaintenanceStatus, POLL_INTERVAL);netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (1)
49-56: Enhance accessibilityConsider adding ARIA attributes to improve accessibility for screen readers.
- <Paper elevation={0} className={classes.banner}> + <Paper + elevation={0} + className={classes.banner} + role="alert" + aria-live="polite" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
netmanager/src/utils/hooks/useMaintenanceStatus.js(1 hunks)netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js(1 hunks)netmanager/src/views/layouts/Main.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager/src/views/layouts/Main.js
🔇 Additional comments (4)
netmanager/src/utils/hooks/useMaintenanceStatus.js (3)
1-8: Clean and well-structured hook initialization!
The state management setup follows React best practices with separate states for data, loading, and error handling.
34-35: Clean hook return value!
The hook follows React conventions by returning all necessary states in a single object.
13-17: Consider adding type checking for maintenance array
The code assumes a specific structure for the maintenance array. Consider adding validation to ensure the data matches the expected format.
if (response.success && response.maintenance?.length > 0) {
- // Get the first active maintenance
- const activeMaintenance = response.maintenance.find((m) => m.isActive);
+ // Validate maintenance data structure
+ const activeMaintenance = response.maintenance.find((m) =>
+ m && typeof m === 'object' &&
+ typeof m.isActive === 'boolean' &&
+ m.isActive
+ );
setMaintenance(activeMaintenance || null);netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (1)
40-46: Well-structured component initialization!
Good use of early return pattern for handling loading and inactive states.
| import { Paper, Typography, Box } from '@material-ui/core'; | ||
| import { Warning as WarningIcon } from '@material-ui/icons'; | ||
| import { useMaintenanceStatus } from 'utils/hooks/useMaintenanceStatus'; | ||
| import { formatDateString } from '../../../utils/dateTime'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import
The formatDateString import is not used in the component. Consider removing it.
-import { formatDateString } from '../../../utils/dateTime';| Estimated downtime: {format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a')} -{' '} | ||
| {format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a')} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date validation
Consider adding error handling for invalid date values to prevent runtime errors.
- Estimated downtime: {format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a')} -{' '}
- {format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a')}
+ Estimated downtime: {
+ maintenance.startDate && !isNaN(new Date(maintenance.startDate))
+ ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a')
+ : 'Invalid start date'
+ } -{' '}
+ {
+ maintenance.endDate && !isNaN(new Date(maintenance.endDate))
+ ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a')
+ : 'Invalid end date'
+ }📝 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.
| Estimated downtime: {format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a')} -{' '} | |
| {format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a')} | |
| Estimated downtime: { | |
| maintenance.startDate && !isNaN(new Date(maintenance.startDate)) | |
| ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid start date' | |
| } -{' '} | |
| { | |
| maintenance.endDate && !isNaN(new Date(maintenance.endDate)) | |
| ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid end date' | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (2)
8-37: Consider consolidating margin styles.The
bannerclass has bothmargin: 5andmarginBottom: theme.spacing(2). This could lead to unexpected styling behavior.banner: { padding: theme.spacing(2), backgroundColor: '#fff3e0', border: '1px solid #ffb74d', borderRadius: '4px', - marginBottom: theme.spacing(2), - margin: 5 + margin: theme.spacing(0.625) },
39-42: Add type checking for maintenance object.Consider adding type checking for the maintenance object to prevent runtime errors.
const MaintenanceBanner = () => { const classes = useStyles(); const { maintenance, loading } = useMaintenanceStatus(); + const isValidMaintenance = maintenance && + typeof maintenance === 'object' && + 'isActive' in maintenance; - if (loading || !maintenance || !maintenance.isActive) { + if (loading || !isValidMaintenance || !maintenance.isActive) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js
[error] 58-58: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 62-62: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (1)
netmanager/src/views/components/MaintenanceBanner/MaintenanceBanner.js (1)
1-7: LGTM! Clean and well-organized imports.
All imports are properly organized and actively used in the component.
| return ( | ||
| <Paper elevation={0} className={classes.banner}> | ||
| <Box className={classes.content}> | ||
| <WarningIcon className={classes.icon} /> | ||
| <Typography variant="body1" className={classes.message}> | ||
| {maintenance.message || | ||
| 'System maintenance in progress. Some features may be unavailable.'} | ||
| </Typography> | ||
| </Box> | ||
| <Typography variant="body2" className={classes.timeInfo}> | ||
| Estimated downtime:{' '} | ||
| {maintenance.startDate && !isNaN(new Date(maintenance.startDate)) | ||
| ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a') | ||
| : 'Invalid start date'}{' '} | ||
| -{' '} | ||
| {maintenance.endDate && !isNaN(new Date(maintenance.endDate)) | ||
| ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a') | ||
| : 'Invalid end date'} | ||
| </Typography> | ||
| </Paper> | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Add ARIA attributes to improve maintenance banner accessibility
The maintenance banner should include ARIA attributes to ensure proper accessibility:
- Add
role="alert"to the Paper component - Add
aria-live="polite"to handle dynamic message updates - Add
aria-atomic="true"to ensure the entire message is read
🔗 Analysis chain
Verify accessibility of the maintenance banner.
While the implementation looks good, let's ensure the maintenance banner meets accessibility standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for accessibility-related attributes in the codebase
# Search for aria-live usage in maintenance-related components
rg -i "aria-live|role=\"alert\"" --type js
# Look for color contrast issues in theme files
rg -i "warning.*color|#fff3e0|#ffb74d|#f57c00|#424242|#616161" --type js
Length of output: 78161
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 62-62: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| {maintenance.startDate && !isNaN(new Date(maintenance.startDate)) | ||
| ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a') | ||
| : 'Invalid start date'}{' '} | ||
| -{' '} | ||
| {maintenance.endDate && !isNaN(new Date(maintenance.endDate)) | ||
| ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a') | ||
| : 'Invalid end date'} |
There was a problem hiding this comment.
Replace global isNaN with Number.isNaN for safer type checking.
The global isNaN performs type coercion which can lead to unexpected results. Use Number.isNaN instead for more precise number validation.
- {maintenance.startDate && !isNaN(new Date(maintenance.startDate))
+ {maintenance.startDate && !Number.isNaN(new Date(maintenance.startDate).getTime())
? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a')
: 'Invalid start date'}{' '}
-{' '}
- {maintenance.endDate && !isNaN(new Date(maintenance.endDate))
+ {maintenance.endDate && !Number.isNaN(new Date(maintenance.endDate).getTime())
? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a')
: 'Invalid end date'}📝 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.
| {maintenance.startDate && !isNaN(new Date(maintenance.startDate)) | |
| ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid start date'}{' '} | |
| -{' '} | |
| {maintenance.endDate && !isNaN(new Date(maintenance.endDate)) | |
| ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid end date'} | |
| {maintenance.startDate && !Number.isNaN(new Date(maintenance.startDate).getTime()) | |
| ? format(new Date(maintenance.startDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid start date'}{' '} | |
| -{' '} | |
| {maintenance.endDate && !Number.isNaN(new Date(maintenance.endDate).getTime()) | |
| ? format(new Date(maintenance.endDate), 'MMM d, yyyy h:mm a') | |
| : 'Invalid end date'} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 62-62: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
Summary of Changes (What does this PR do?)
This pull request introduces a new feature for displaying a maintenance banner, along with some refactoring and improvements to the user list pagination logic. Below are the most important changes:
New Feature: Maintenance Banner
GET_MAINTENANCE_STATUSto theauthService.jsconfiguration file to define the endpoint for fetching maintenance status.useMaintenanceStatusinuseMaintenanceStatus.jsto fetch and manage the maintenance status.getMaintenanceStatusApiinauthService.jsto call the maintenance status endpoint.MaintenanceBannerinMaintenanceBanner.jsto display a banner during maintenance periods.MaintenanceBannercomponent into the main layout inMain.js. [1] [2]Refactoring: User List Pagination
pageSizeandcurrentPagestate variables withlimitandskipinAvailableUserList.jsto simplify pagination logic. [1] [2] [3]UserList.jsto uselimitandskipfor pagination. [1] [2] [3] [4]Code Cleanup
isEmptyimport fromAvailableUserList.js.Sidebar.jsto improve readability.Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates aim to improve user experience by providing clear maintenance notifications and optimizing user management functionalities.