[Netmanager] Code cleanup and refactoring #2288
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14679479 | Triggered | Bearer Token | f03523b | netmanager/src/views/pages/ReportTemplate/Main.js | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes the removal of several components and associated tests from the project, specifically components related to AirQlouds, as well as modifications to various files to replace local storage retrievals with Redux state management. The changes enhance the application's structure by ensuring that the active network state is consistently managed through Redux, improving overall state management and reducing reliance on local storage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Redux
User->>App: Interacts with App
App->>Redux: Fetches activeNetwork
Redux-->>App: Returns activeNetwork
App->>User: Displays data based on activeNetwork
Possibly related PRs
Suggested labels
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: 32
🧹 Outside diff range and nitpick comments (73)
netmanager/src/views/pages/Map/components/Indicator.js (1)
84-120: Consider these accessibility and maintainability improvements
- Add ARIA labels to enhance accessibility:
<div className="indicator"> + <div role="group" aria-label="Air Quality Indicators"> {/* existing content */} + </div> </div>
- Consider extracting repeated values into constants:
const ICON_DIMENSIONS = { width: 30, height: 30 }; const AIR_QUALITY_LEVELS = [ { label: 'Good', text: 'Air is clean and poses no health risks.', icon: Good, fill: '#45e50d', className: 'good' }, // ... other levels ];This would make the component more maintainable and reduce repetition.
netmanager/src/views/pages/Map/OverlayMap.js (4)
161-162: Consider using object shorthand notation for better readability.The MenuItem onClick handlers use verbose object literals. You can simplify them using object shorthand notation.
-onClick={handleMenuItemChange('pm2_5', { - pm2_5: true, - no2: false, - pm10: false -})} +onClick={handleMenuItemChange('pm2_5', { pm2_5: true, no2: false, pm10: false })}Also applies to: 171-172, 181-182
234-235: Consider memoizing event handlers to prevent unnecessary re-renders.The onMouseEnter/onMouseLeave and onClick handlers are recreated on every render. Consider using useCallback for better performance.
+const handleMouseEnter = useCallback(() => handleHover(true), []); +const handleMouseLeave = useCallback(() => handleHover(false), []); <div ref={dropdownRef} className="map-style-placeholder" onClick={handleClick} - onMouseEnter={() => handleHover(true)} - onMouseLeave={() => handleHover(false)} + onMouseEnter={handleMouseEnter} + onMouseLeave={handleMouseLeave} >Also applies to: 290-291
Line range hint
1-650: Consider migrating from localStorage to Redux for consistent state management.The code heavily relies on localStorage for state management (pollutant, mapMode, mapStyle, etc.). Since the PR objectives mention transitioning to Redux hooks for state management, consider:
- Moving these values to Redux store
- Using useSelector/useDispatch hooks
- Removing direct localStorage access
This would:
- Improve state predictability
- Enable easier state debugging
- Prevent potential race conditions
- Align with the PR's state management goals
Line range hint
1-650: Add error boundaries for map initialization and data loading.While there's error handling for individual operations, consider wrapping critical map operations in dedicated error boundaries:
- Map initialization
- Data loading/processing
- Marker creation
This would:
- Prevent entire component crashes
- Provide better error recovery
- Improve user experience during failures
netmanager/src/views/components/SIM/SimRegistry.js (2)
Line range hint
214-218: Consider consolidating loading states and improving error handling consistencyA few suggestions for improving the code:
- The component uses both
isLoadingandloadingstates. Consider consolidating these into a single loading state for better maintainability.- Error handling alternates between direct state updates and dispatch calls. Consider standardizing the approach.
Example consolidation:
- const [isLoading, setIsLoading] = useState(false); - const [loading, setLoading] = useState(false); + const [isLoading, setIsLoading] = useState(false);Then update all loading state references to use the consolidated state.
Also applies to: 267-268
Line range hint
253-253: Improve handling of disabled delete functionalityThe
handleDeletefunction is empty but exposed in the UI with a disabled button. Consider either:
- Removing the delete button entirely if the functionality isn't implemented yet
- Adding a tooltip message explaining why deletion is disabled
- <Tooltip title="Delete" placement="bottom" arrow> + <Tooltip title="Delete functionality is currently disabled" placement="bottom" arrow>Also applies to: 332-336
netmanager/src/views/pages/Analytics/components/CohortsDashboard.jsx (2)
Line range hint
29-32: Remove duplicate style definition.The
chartContainerstyle is defined twice in theuseStyleshook with identical properties. This could lead to maintenance confusion.const useStyles = makeStyles((theme) => ({ chartCard: {}, chartContainer: { minHeight: 250, position: 'relative' }, customChartCard: { width: '100%', padding: '20px', minHeight: '200px' }, differenceIcon: { color: theme.palette.text.secondary }, - chartContainer: { - minHeight: 250, - position: 'relative' - }, actions: { justifyContent: 'flex-end' } }));Also applies to: 34-37
Line range hint
98-98: Remove commented console.log.Since we're using version control, we can safely remove commented-out debug statements. This helps maintain a cleaner codebase.
- // console.log(initialCount);netmanager/src/views/pages/GridsRegistry/AddGridForm.jsx (2)
20-41: Well-structured error handling utility! Consider adding JSDoc.The
extractErrorMessagefunction handles different error shapes elegantly with proper null checks and fallbacks.Consider adding JSDoc for better documentation:
+/** + * Extracts error message from API error response + * @param {Error} error - The error object from API response + * @returns {string} Formatted error message + */ const extractErrorMessage = (error) => {
207-212: Consider extracting form validation logic.The submit button's disabled condition handles all necessary validations but could be more readable.
Consider extracting the validation logic:
+const isFormValid = () => { + return !loading && polygon && form.name && form.admin_level && form.network; +}; <Button onClick={onSubmit} color="primary" variant="contained" - disabled={loading || !polygon || !form.name || !form.admin_level || !form.network} + disabled={!isFormValid()} >netmanager/src/views/components/DataDisplay/DeviceManagement/AirqloudUptimeChart.js (2)
30-30: Consider adding fallback handling for undefined networkThe Redux selector is a good improvement over localStorage, aligning with the PR's state management goals. However, consider adding a default value or error state handling for cases where the network might be undefined.
- const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); + const activeNetwork = useSelector((state) => state.accessControl.activeNetwork) || null; + const [error, setError] = useState(null); + + useEffect(() => { + if (!activeNetwork) { + setError('No active network selected'); + } else { + setError(null); + } + }, [activeNetwork]);
Line range hint
82-124: Consider extracting date validation logic into a custom hookThe date validation logic in
resetAirqloudUptimeChartcould be extracted into a custom hook for better reusability and maintainability. This would also make the component more focused on its primary responsibility of chart rendering.Example structure:
// useDateValidation.js const useDateValidation = (startDate, endDate, maxDays = 7) => { const [error, setError] = useState(''); const validateDates = useCallback(() => { if (new Date(startDate) > new Date(endDate)) { return 'End date is older than start date. Please adjust.'; } const diffInDays = Math.floor( (new Date(endDate) - new Date(startDate)) / (1000 * 60 * 60 * 24) ); if (diffInDays > maxDays) { return `Time period should be less than ${maxDays} days`; } return ''; }, [startDate, endDate, maxDays]); return { error, validateDates }; };netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js (1)
Line range hint
46-77: Consider optimizing the loading state managementThe function has a redundant
setLoading(false)call outside the promise chain. This could lead to race conditions where the loading state is cleared before the API call completes.Here's a suggested improvement:
const submitEditUser = (user) => { setLoading(true); const userID = user._id; // assign user to network if (!isEmpty(activeNetwork) && !isEmpty(userID)) { assignUserNetworkApi(activeNetwork._id, userID) .then((response) => { dispatch(fetchAvailableNetworkUsers(activeNetwork._id)); dispatch( updateMainAlert({ message: 'User successfully added to the organisation. Check in assigned users table', show: true, severity: 'success' }) ); setLoading(false); }) .catch((error) => { const errors = error.response && error.response.data && error.response.data.errors; dispatch( updateMainAlert({ message: error.response && error.response.data && error.response.data.message, show: true, severity: 'error', extra: createAlertBarExtraContentFromObject(errors || {}) }) ); setLoading(false); }); - } - setLoading(false); + } else { + setLoading(false); + } };netmanager/src/views/components/Sites/SiteView.js (1)
53-53: Enhance error handling in handleSubmit functionWhile the Redux integration looks good, the error handling could be more robust. Consider these improvements:
.catch((err) => { - const errors = (err.response && err.response.data && err.response.data.error) || {}; + const errors = err?.response?.data?.error || {}; setErrors(errors); dispatch( updateMainAlert({ severity: 'error', - message: err.response && err.response.data.message, + message: err?.response?.data?.message || 'An error occurred while updating the site', show: true }) ); });Also applies to: 89-103
netmanager/src/views/pages/GridsRegistry/GridsDetails.js (2)
Line range hint
12-24: Review discount calculation logicThe current implementation adds a flat $20 fee after applying the discount, which could result in a higher final price than the original amount for small purchases. For example:
- Original amount: $100
- 10% discount: -$10
- Flat fee: +$20
- Final amount: $110 (higher than original)
Consider one of these alternatives:
- Apply the flat fee before the discount
- Make the flat fee proportional to the purchase amount
- Set a minimum purchase amount for the discount to apply
Would you like me to help implement any of these alternatives?
147-148: Clean up unnecessary style spreadMultiple style objects are being defined inline with single properties. Consider consolidating these styles into the useStyles hook for better maintainability.
const useStyles = makeStyles((theme) => ({ root: { padding: theme.spacing(4) }, + container: { + width: '96%', + margin: '20px auto' + }, + innerContainer: { + margin: '50px auto', + maxWidth: '1500px' + }, + card: { + overflow: 'visible' + } // ... other styles }));Also applies to: 156-157, 163-164, 222-223, 229-230, 237-238
netmanager/src/views/components/Grid/GridReportsView.js (2)
Line range hint
57-799: Consider breaking down the large componentThe GridReportsView component is handling too many responsibilities:
- Data summary report generation
- Uptime report generation
- Multiple table displays
- Complex state management
Consider splitting this into smaller, focused components:
- GridDataSummaryReport
- GridUptimeReport
- GridReportForm (reusable for both report types)
This would improve:
- Code maintainability
- Component reusability
- Testing capabilities
- State management
Would you like me to help with this refactoring?
286-287: Simplify button disable logicThe disable logic for both report buttons can be simplified using object destructuring.
- const disableUptimeReportGenerationBtn = () => { - return !(uptimeStartDate && uptimeEndDate) || loading; - }; + const disableUptimeReportGenerationBtn = () => + !uptimeStartDate || !uptimeEndDate || loading;Also applies to: 577-578
netmanager/src/views/pages/UserList/components/UsersToolbar/UsersToolbar.js (2)
Line range hint
142-155: Add null check for activeNetwork in initialStateThe form initialization assumes activeNetwork is always present. Consider adding a fallback for cases where activeNetwork might be null or undefined:
const initialState = { firstName: '', lastName: '', email: '', role: {}, errors: {}, privilege: 'user', country: '', jobTitle: '', website: '', - long_organization: activeNetwork.net_name, - organization: activeNetwork.net_name + long_organization: activeNetwork?.net_name || '', + organization: activeNetwork?.net_name || '' };
Line range hint
234-283: Consider optimizing the user registration flowThe current implementation makes sequential API calls (addUserApi → assignUserNetworkApi → assignUserToRoleApi) with separate error handling blocks. Consider:
- Using Promise.all for parallel API calls where possible
- Implementing a rollback mechanism for partial failures
- Consolidating error handling logic
Example approach:
const registerUser = async (userData) => { try { const { user } = await addUserApi(userData); await Promise.all([ assignUserNetworkApi(activeNetwork._id, user._id), assignUserToRoleApi(userData.role.id, { user: user._id }) ]); return user; } catch (error) { // Consolidated error handling handleRegistrationError(error); throw error; } };netmanager/src/views/components/Hosts/HostView.js (4)
13-13: Consider adding error handling for undefined activeNetwork stateThe Redux integration looks good, but consider adding a fallback for cases where activeNetwork might be undefined.
-const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); +const activeNetwork = useSelector((state) => state.accessControl.activeNetwork) || {};Also applies to: 133-133, 599-599
Line range hint
232-242: Consider increasing error message display durationThe current 1500ms duration might be too short for users to read longer error messages. Consider increasing it or making it proportional to the message length.
- }, 1500); + }, Math.max(2000, errorMessage.length * 50));
482-483: Enhance dialog accessibilityWhile the addition of aria-describedby is good, consider adding more ARIA attributes for better screen reader support.
<Dialog open={mobileMoneyDialog} onClose={handleCloseDialog} aria-labelledby="form-dialog-title" aria-describedby="form-dialog-description" + aria-modal="true" + role="dialog" >Also applies to: 527-528
Line range hint
482-577: Consider adding retry mechanism for failed transactionsThe mobile money transaction handling could benefit from a retry mechanism in case of temporary network issues.
Consider implementing an exponential backoff retry strategy for failed transactions, with a maximum of 3 attempts.
netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx (5)
Line range hint
93-94: Remove commented console.logRemove the commented console.log statement to maintain clean code.
- // console.log(initialCount);
Line range hint
108-127: Consider using date-fns for date formattingThe current date formatting implementation using manual string concatenation and leading zeros could be simplified using a date formatting library like date-fns.
- function appendLeadingZeroes(n) { - if (n <= 9) { - return '0' + n; - } - return n; - } - - let todaysDate = new Date(); - const dateValue = appendLeadingZeroes( - todaysDate.getDate() + - '/' + - appendLeadingZeroes(todaysDate.getMonth() + 1) + - '/' + - todaysDate.getFullYear() - ); + import { format } from 'date-fns'; + const dateValue = format(new Date(), 'dd/MM/yyyy');
Line range hint
44-52: Consider extracting PM2.5 categories to constantsThe initial state for pm2_5SiteCount could be derived from the PM_25_CATEGORY constant to ensure consistency and reduce duplication.
- const [pm2_5SiteCount, setPm2_5SiteCount] = useState({ - Good: 0, - Moderate: 0, - UHFSG: 0, - Unhealthy: 0, - VeryUnhealthy: 0, - Hazardous: 0 - }); + const [pm2_5SiteCount, setPm2_5SiteCount] = useState( + Object.keys(PM_25_CATEGORY).reduce((acc, category) => ({ ...acc, [category]: 0 }), {}) + );
Line range hint
39-39: Add prop validation for grid and gridDetailsConsider adding PropTypes or TypeScript types to validate the component's props.
+import PropTypes from 'prop-types'; const GridsDashboardView = ({ grid, gridDetails, loading }) => { // ... component implementation ... }; +GridsDashboardView.propTypes = { + grid: PropTypes.shape({ + _id: PropTypes.string.isRequired, + name: PropTypes.string, + numberOfSites: PropTypes.number + }).isRequired, + gridDetails: PropTypes.shape({ + long_name: PropTypes.string, + admin_level: PropTypes.string, + numberOfSites: PropTypes.number, + visibility: PropTypes.string, + sites: PropTypes.array + }), + loading: PropTypes.bool +};
Line range hint
132-133: Consider handling errors in data fetchingThe useEffect hook for loading user default graph data should include error handling.
useEffect(() => { - dispatch(loadUserDefaultGraphData(true, grid._id)); + try { + dispatch(loadUserDefaultGraphData(true, grid._id)); + } catch (error) { + console.error('Failed to load user default graph data:', error); + // Consider adding error state and UI feedback + } }, [grid]);netmanager/src/views/pages/ExportData/index.js (3)
Line range hint
437-454: Enhance date validation logicThe date validation could be more robust and the
MAX_ALLOWED_DATE_DIFF_IN_DAYSconstant is defined but not used.Consider implementing these improvements:
const Difference_In_Time = new Date(endDate).getTime() - new Date(startDate).getTime(); const Difference_In_Days = Difference_In_Time / (1000 * 3600 * 24); -if (Difference_In_Days > 28) { +if (Difference_In_Days > MAX_ALLOWED_DATE_DIFF_IN_DAYS) { setSnackbarMessage( - 'For time periods greater than a month, please reduce the time difference to a week to avoid timeouts!' + `For time periods greater than ${MAX_ALLOWED_DATE_DIFF_IN_DAYS} days, please reduce the time difference to avoid timeouts!` ); handleClickSnackbar(); + setLoading(false); + return; }
Line range hint
533-603: Reduce code duplication in form handlingThe form submission logic is duplicated across all four tabs with minimal differences.
Consider extracting the common form logic into a reusable component:
const ExportDataForm = ({ type, options, selectedItems, onItemsChange, onSubmit }) => { // Common form fields and logic return ( <form onSubmit={onSubmit}> {/* Common form fields */} </form> ); };This would reduce code duplication and make maintenance easier.
Also applies to: 604-674, 675-745, 746-816
Line range hint
394-433: Improve error handling in data exportThe error handling in
downloadDataFunccould be more specific and informative.Consider these improvements:
.catch((err) => { - if (err.response.data.status === 'success') { + if (!err.response) { + dispatch( + updateMainAlert({ + message: 'Network error occurred while downloading data', + show: true, + severity: 'error' + }) + ); + } else if (err.response.data.status === 'success') { dispatch( updateMainAlert({ message: 'Uh-oh! No data found for the selected parameters.', show: true, - severity: 'success' + severity: 'info' }) ); } else { dispatch( updateMainAlert({ - message: err.response.data.message, + message: `Error downloading data: ${err.response.data.message}`, show: true, severity: 'error' }) ); }netmanager/src/views/pages/UserList/AvailableUserList.js (1)
37-40: Simplify error handling with optional chainingThe error handling can be simplified using optional chaining.
- let errorMessage = 'An error occurred'; - if (error.response && error.response.data && error.response.data.message) { - errorMessage = error.response.data.message; - } + const errorMessage = error.response?.data?.message || 'An error occurred';🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/views/pages/CohortsRegistry/index.js (1)
44-44: Consider handling network switch scenariosWhile moving to Redux selector is great, we should ensure proper handling of network switching. Consider adding a dependency on
activeNetworkin the second useEffect to refresh device options when the network changes.useEffect(() => { if (!isEmpty(devices)) { const deviceOptions = createDeviceOptions(Object.values(devices)); setDeviceOptions(deviceOptions); } - }, [devices]); + }, [devices, activeNetwork]);netmanager/src/views/pages/Roles/components/RolesToolbar.js (2)
Line range hint
134-157: Consider standardizing error handling approach.The error handling could be more consistent between the two catch blocks. The first catch block uses a generic message while the second one attempts to access the specific error message from the response.
Consider applying this more robust error handling approach:
.catch((error) => { dispatch( updateMainAlert({ - message: 'Unable to create new role. Please try again.', + message: error.response?.data?.message || 'Unable to create new role. Please try again.', show: true, severity: 'error' }) ); setLoading(false); });
Line range hint
108-133: Consider separating role creation and permission assignment logic.The onSubmit function currently handles both role creation and permission assignment in a nested promise chain. This could be simplified and made more maintainable by separating these concerns.
Consider:
- Creating a separate service function that handles both operations
- Using async/await for better readability
- Implementing proper rollback in case permission assignment fails after role creation
Example approach:
const createRoleWithPermissions = async (roleData, permissions) => { try { const role = await addRoleApi(roleData); await assignPermissionsToRoleApi(role.created_role._id, { permissions }); return role; } catch (error) { // Handle rollback if needed throw error; } };netmanager/src/views/components/Hosts/HostsTable.js (4)
13-13: Nice work on migrating to Redux state management! 👍The transition from localStorage to Redux's useSelector for managing activeNetwork state is a solid architectural improvement. This change enhances state consistency and makes the component more maintainable.
Also applies to: 103-103
273-274: Consider enhancing phone number validationWhile the UI styling looks good, the phone number validation could be strengthened. Currently, there's no format validation for the "+256xxxxxxxxx" pattern shown in the placeholder.
const handleHostChange = (prop, index) => (event) => { if (prop === 'phone_numbers') { + const phoneRegex = /^\+256\d{9}$/; const phoneNumbers = [...host.phone_numbers]; phoneNumbers[index] = event.target.value; + const phoneErrors = [...errors.phone_numbers]; + phoneErrors[index] = !phoneRegex.test(event.target.value) ? 'Invalid phone number format' : ''; setHost({ ...host, phone_numbers: phoneNumbers }); + setErrors({ ...errors, phone_numbers: phoneErrors }); } else { setHost({ ...host, [prop]: event.target.value }); } - setErrors({ ...errors, [prop]: '' }); };
288-289: Good start on accessibility, let's enhance it furtherWhile adding aria-describedby is great, consider adding more descriptive labels for screen readers.
<Dialog open={addHostDialog} onClose={handleCloseDialog} aria-labelledby="form-dialog-title" aria-describedby="form-dialog-description" + role="dialog" + aria-modal="true" >
365-366: Consider moving inline styles to useStylesFor better maintainability and consistency, consider moving the button margin to the useStyles definition.
const useStyles = makeStyles((theme) => ({ // ... other styles + actionButton: { + margin: theme.spacing(0, 2) + } })); // Then in the JSX -<Button style={{ margin: '0 15px' }} /> +<Button className={classes.actionButton} />netmanager/src/views/layouts/common/Sidebar/Sidebar.js (3)
242-250: Consider adding loading states for Redux selectorsThe Redux selectors might return undefined during initial render. Consider adding null checks or default values to prevent potential rendering issues.
- const currentRole = useSelector((state) => state.accessControl.currentRole); - const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); - const userNetworks = useSelector((state) => state.accessControl.userNetworks); + const currentRole = useSelector((state) => state.accessControl.currentRole) || null; + const activeNetwork = useSelector((state) => state.accessControl.activeNetwork) || null; + const userNetworks = useSelector((state) => state.accessControl.userNetworks) || [];
Line range hint
264-336: Refactor complex effect hook for better maintainabilityThe current implementation has several issues:
- The effect is too large and handles multiple responsibilities
- Deep nesting makes the code hard to follow
- Duplicate code in the else branches
Consider breaking this into smaller, focused functions:
// Extract page filtering logic const getFilteredPages = (role) => { if (!isEmpty(role) && role.role_permissions) { const accessDeniedForUserPages = checkAccess(allMainPages, role.role_permissions); const accessDeniedForAdminPages = checkAccess(allUserManagementPages, role.role_permissions); return { userPages: excludePages(allMainPages, accessDeniedForUserPages), adminPages: excludePages(allUserManagementPages, accessDeniedForAdminPages) }; } return { userPages: excludePages(allMainPages, [ 'Locate', 'Network Monitoring', // ... other excluded pages ]), adminPages: excludePages(allUserManagementPages, [ 'Users', 'Candidates', // ... other excluded pages ]) }; }; // Simplified effect useEffect(() => { const currentUser = JSON.parse(localStorage.getItem('currentUser')); if (isEmpty(currentUser)) { return; } const { userPages, adminPages } = getFilteredPages(currentRole); setUserPages(userPages); setAdminPages(adminPages); setLoading(false); }, [currentRole, activeNetwork, userNetworks]);
336-336: Optimize effect dependenciesThe effect depends on
activeNetworkanduserNetworksbut doesn't use them directly. Consider removing these dependencies to prevent unnecessary re-renders.- }, [currentRole, activeNetwork, userNetworks]); + }, [currentRole]); // Only depend on currentRole since it's the only value usednetmanager/src/views/pages/CohortsRegistry/AddCohortForm.jsx (1)
Line range hint
71-93: Consider refactoring duplicate error handling.The error handling logic is duplicated in both catch blocks. Consider extracting it into a reusable function.
Here's a suggested refactor:
+ const handleError = (error) => { + const errors = error.response?.data?.errors; + handleClose(); + dispatch( + updateMainAlert({ + show: true, + message: error.response?.data?.message, + severity: 'error', + extra: createAlertBarExtraContentFromObject(errors || {}) + }) + ); + setLoading(false); + }; // Then in your catch blocks: - .catch((error) => { - const errors = error.response && error.response.data && error.response.data.errors; - handleClose(); - dispatch( - updateMainAlert({ - show: true, - message: error.response && error.response.data && error.response.data.message, - severity: 'error', - extra: createAlertBarExtraContentFromObject(errors || {}) - }) - ); - setLoading(false); - }); + .catch(handleError);This refactor would:
- Reduce code duplication
- Make the error handling more maintainable
- Use optional chaining for cleaner null checks
netmanager/src/views/pages/Map/components/Filter.jsx (2)
Line range hint
121-144: Consider extracting pollutant configurations and optimizing state management.The component could benefit from the following improvements:
- Extract pollutant configurations into a separate constant
- Consider using a shared configuration object for the Scale components
- Optimize state management to prevent unnecessary re-renders
Here's a suggested approach:
const POLLUTANT_CONFIGS = { pm2_5: { title: 'PM2.5 AQI', ranges: { good: '0 - 12', moderate: '12.1 - 35.4', // ... other ranges } }, // ... other pollutants }; const Filter = ({ pollutants }) => { const [open, setOpen] = useState(false); const renderScale = (pollutantKey) => { const config = POLLUTANT_CONFIGS[pollutantKey]; return ( <Scale key={pollutantKey} title={ <span style={{ color: '#3067e2', cursor: 'pointer' }}> {config.title} </span> } isFilterOpen={open} toggleFilter={() => setOpen(!open)} {...config.ranges} /> ); }; return ( <> {Object.keys(pollutants) .filter(key => pollutants[key]) .map(renderScale)} </> ); };
ReactTooltip is stable, but pollutant validation could be improved
The ReactTooltip v4.2.9 is a stable version and not deprecated. However, the pollutant validation could be enhanced:
- The Filter component currently relies on truthiness checks (
pollutants.pm2_5 && ...) which may not catch all invalid cases- Consider adding explicit validation using the existing error handling from MapNodes.js where invalid pollutants are properly caught
🔗 Analysis chain
Line range hint
1-2: Verify ReactTooltip dependency and add error handling.
- The
react-tooltippackage might be deprecated. Consider verifying the version and checking for alternatives.- Consider adding error handling for invalid pollutant values.
Let me verify the ReactTooltip dependency and error handling concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for react-tooltip version rg -g 'package.json' '"react-tooltip":' # Look for existing error handling patterns in the codebase rg -g '*.{js,jsx,ts,tsx}' '(try|catch|error|invalid|isValid)'Length of output: 98030
Script:
#!/bin/bash # Check ReactTooltip version and deprecation status rg -g 'package.json' '"react-tooltip":' # Look for error handling patterns in the Filter component cat netmanager/src/views/pages/Map/components/Filter.jsx # Check for pollutant validation in Filter component rg -g '*.{js,jsx}' 'pollutant.*valid|valid.*pollutant'Length of output: 6041
netmanager/src/views/components/Sites/SiteToolbar.js (2)
353-354: Consider enhancing form validationWhile the basic validation is in place, consider adding:
- Latitude/longitude range validation (-90 to 90 for lat, -180 to 180 for long)
- Input sanitization for the site name
const handleSiteDataChange = (key) => (event) => { + const value = event.target.value; if (key === 'phoneNumber') { let re = /\s*|\d+(\.d+)?/; if (!re.test(value)) { return; } } + // Add validation for latitude/longitude + if (key === 'latitude') { + if (value && (value < -90 || value > 90)) { + setErrors(prev => ({...prev, latitude: 'Latitude must be between -90 and 90'})); + return; + } + } + if (key === 'longitude') { + if (value && (value < -180 || value > 180)) { + setErrors(prev => ({...prev, longitude: 'Longitude must be between -180 and 180'})); + return; + } + } return setSiteData({ ...siteData, [key]: event.target.value }); };Also applies to: 336-337
424-425: Add error handling for map previewThe map preview functionality should handle cases where the map service is unavailable.
const openMap = (siteMetaData) => { + try { window.open( `${mapview}/?mlat=${siteMetaData.latitude}&mlon=${siteMetaData.longitude}&zoom=8`, '_blank' ); + } catch (error) { + dispatch( + updateMainAlert({ + message: 'Unable to open map preview. Please try again later.', + show: true, + severity: 'error' + }) + ); + } };Also applies to: 435-436
netmanager/src/views/pages/CohortsRegistry/CohortDetails.jsx (1)
358-359: Consider using a custom select component for consistencyThe native select for visibility might not match the UI design system. Consider using OutlinedSelect component that's already being used elsewhere in the file.
- <TextField - select - id="visibility" - label="Visibility" - variant="outlined" - value={form.visibility} - fullWidth - SelectProps={{ - native: true, - style: { width: '100%', height: '53px' } - }} - required - InputLabelProps={{ shrink: true }} - onChange={handleSelectFieldChange('visibility')} - > - <option value={true}>True</option> - <option value={false}>False</option> - </TextField> + <OutlinedSelect + fullWidth + name="visibility" + label="Visibility" + value={form.visibility ? { value: true, label: 'True' } : { value: false, label: 'False' }} + options={[ + { value: true, label: 'True' }, + { value: false, label: 'False' } + ]} + onChange={(option) => handleSelectFieldChange('visibility')({ target: { value: option.value } })} + variant="outlined" + margin="dense" + required + />netmanager/src/views/components/DataDisplay/DeviceView/DeviceDeployStatus.js (4)
Line range hint
300-359: Improve RecallButton implementation for better UX and reliabilityThe RecallButton component has several areas for improvement:
- Avoid using
window.location.reload()as it creates a jarring user experience- Potential race condition between
setSelectVisible(false)andsetrecallLoading(true)- Missing cleanup for component state on unmount
Consider this improved implementation:
const RecallButton = ({ handleRecall, recallLoading }) => { const [selectedRecallType, setSelectedRecallType] = useState(''); const [selectVisible, setSelectVisible] = useState(false); + const [isSubmitting, setIsSubmitting] = useState(false); + + useEffect(() => { + return () => { + // Cleanup on unmount + setSelectedRecallType(''); + setSelectVisible(false); + }; + }, []); const handleRecallClick = async () => { + if (isSubmitting) return; if (!selectVisible) { setSelectVisible(true); return; } if (selectedRecallType) { + setIsSubmitting(true); try { await handleRecall(selectedRecallType); - window.location.reload(); + // Let the parent component handle the success } catch (error) { console.error('Recall failed:', error); + } finally { + setIsSubmitting(false); + setSelectVisible(false); + setSelectedRecallType(''); } - setrecallLoading(false); - setSelectedRecallType(''); } };
Line range hint
479-553: Security and reliability concerns in handleDeploySubmitThe current implementation has several security and reliability issues:
- Unsafe direct access to localStorage without proper validation
- Sensitive user data (email, name) exposed in deployment payload
- Page reload used for state refresh
Consider these security-focused improvements:
const handleDeploySubmit = async () => { setDeployLoading(true); dispatch(setLoading(true)); try { if (checkErrors()) { setInputErrors(true); return; } - const storedData = localStorage.getItem('currentUser'); - if (!storedData) { - console.error('Error: No user data found in local storage'); - return; - } - const parsedData = JSON.parse(storedData); + // Use Redux selector for user data instead of localStorage + const userData = useSelector(state => state.auth.user); + if (!userData) { + throw new Error('User not authenticated'); + } const deployData = { mountType: installationType, height: height, powerType: power, date: new Date(deploymentDate).toISOString(), isPrimaryInLocation: primaryChecked, isUsedForCollocation: collocationChecked, site_id: site.value, - userName: parsedData.email, - email: parsedData.email, - firstName: parsedData.firstName, - lastName: parsedData.lastName + user_id: userData.id // Only send necessary user identifier }; - if (parsedData._id) { - deployData.user_id = parsedData._id; - } const response = await deployDeviceApi(deviceData.name, deployData); dispatch(updateMainAlert({ message: response.message, show: true, severity: 'success' })); setDeployed(true); setInputErrors(false); - setTimeout(() => { - window.location.reload(); - }, 2000); + // Dispatch action to refresh device data instead of page reload + dispatch(loadDevicesData()); } catch (err) { handleDeployError(err); } finally { setDeployLoading(false); dispatch(setLoading(false)); setIsLoading(false); } };
Line range hint
419-478: Improve form validation and error handlingThe current form validation implementation is scattered and repetitive. Consider using a form management library like Formik or react-hook-form for better maintainability and reliability.
Here's a suggested approach using react-hook-form:
+import { useForm } from 'react-hook-form'; +import * as yup from 'yup'; + +const validationSchema = yup.object({ + height: yup.number().required('Height is required'), + powerType: yup.string().required('Power type is required'), + mountType: yup.string().required('Mount type is required'), + site_id: yup.string().required('Site is required') +}); + const DeviceDeployStatus = ({ deviceData, siteOptions }) => { - const [errors, setErrors] = useState({}); - const [inputErrors, setInputErrors] = useState(false); + const { register, handleSubmit, formState: { errors } } = useForm({ + resolver: yupResolver(validationSchema), + defaultValues: { + height: deviceData.height || '', + powerType: deviceData.powerType || '', + mountType: deviceData.mountType || '', + site_id: deviceData.site?._id || '' + } + }); - const checkErrors = () => { - // Remove existing error checking logic - };This approach provides:
- Centralized validation logic
- Better type safety
- Reduced boilerplate
- Automatic form state management
Line range hint
1-24: Consider breaking down the component for better maintainabilityThe DeviceDeployStatus component is handling too many responsibilities, making it harder to maintain and test. Consider splitting it into smaller, focused components:
Suggested component structure:
- DeviceDeployForm - Handle form inputs and validation
- DeviceTestPanel - Manage device testing functionality
- RecallButton (already separated)
- DeploymentStatusHeader - Display status and actions
This separation would:
- Improve code organization
- Make testing easier
- Enable better state management
- Reduce component complexity
Would you like me to help create these component structures?
netmanager/src/views/containers/PageAccess.js (2)
7-7: Consider adding error handling for the Redux state.While the Redux integration is a good step towards centralized state management, consider adding error handling or type checking for the Redux state to make the code more robust.
- let currentRole = useSelector((state) => state.accessControl.currentRole); + let currentRole = useSelector((state) => { + try { + return state?.accessControl?.currentRole || null; + } catch (error) { + console.error('Error accessing Redux state:', error); + return null; + } + });
Line range hint
5-29: Consider performance optimization and documentation improvements.The HOC implementation could benefit from the following improvements:
- Use
useMemofor permission checking to prevent unnecessary recalculations- Add JSDoc documentation explaining the Redux-first approach with localStorage fallback
Example implementation:
export const withPermission = (Component, requiredPermission) => { const WithPermission = (props) => { let currentRole = useSelector((state) => state.accessControl.currentRole); - let hasPermission = false; + const hasPermission = React.useMemo(() => { + if (!currentRole) { + try { + const storedRole = localStorage.getItem('currentUserRole'); + currentRole = storedRole ? JSON.parse(storedRole) : null; + } catch (error) { + console.error('Error parsing role from localStorage:', error); + return false; + } + } - if (!currentRole) { - currentRole = JSON.parse(localStorage.getItem('currentUserRole')); - } - - if (currentRole && currentRole.role_permissions) { - hasPermission = currentRole.role_permissions.some( - (permission) => permission.permission === requiredPermission - ); - } + return currentRole?.role_permissions?.some( + (permission) => permission.permission === requiredPermission + ) || false; + }, [currentRole, requiredPermission]);Also, consider adding documentation:
/** * Higher-Order Component for permission-based routing * @param {React.ComponentType} Component - The component to wrap * @param {string} requiredPermission - The required permission to access the component * @returns {React.ComponentType} - Protected component that checks permissions using Redux first, then localStorage as fallback */🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/views/pages/UserList/UserList.js (1)
30-31: Consider optimizing useEffect hooks.A few suggestions to improve the code:
- Add dependencies to useEffect hooks to prevent stale closures
- Consider consolidating the two useEffect hooks since they have similar conditions
Here's a suggested refactor:
- useEffect(() => { - if (!activeNetwork) return; - dispatch(loadRolesSummary(activeNetwork._id)); - }, []); - - useEffect(() => { - if (!activeNetwork) return; - dispatch(fetchNetworkUsers(activeNetwork._id)); - }, []); + useEffect(() => { + if (!activeNetwork) return; + + dispatch(loadRolesSummary(activeNetwork._id)); + dispatch(fetchNetworkUsers(activeNetwork._id)); + }, [activeNetwork, dispatch]);Also applies to: 35-36
netmanager/src/views/pages/CohortsRegistry/CohortsTable.js (1)
14-18: Consider adding activeNetwork to useEffect dependenciesThe useEffect hook uses activeNetwork but doesn't include it in the dependency array. While the current implementation works, it might miss updates if activeNetwork changes during the component's lifecycle.
- }, [cohortsList]); + }, [cohortsList, activeNetwork]);netmanager/src/views/components/Activities/ActivitiesTable.js (1)
Line range hint
30-44: Simplify the useEffect logicThe current nesting of conditions makes the code harder to follow. Consider restructuring for better readability and maintainability.
Here's a cleaner approach:
useEffect(() => { if (!activeNetwork) return; if (!isEmpty(site_activities)) return; setIsLoading(true); - if (!isEmpty(activeNetwork)) { - dispatch(loadActivitiesData(activeNetwork.net_name)) - .then(() => { - setIsLoading(false); - }) - .catch((error) => { - console.error(error); - setIsLoading(false); - }); - } else { - setIsLoading(false); - } + dispatch(loadActivitiesData(activeNetwork.net_name)) + .then(() => { + setIsLoading(false); + }) + .catch((error) => { + console.error('Failed to load activities:', error); + setIsLoading(false); + }); - }, []); + }, [dispatch, activeNetwork, site_activities]);netmanager/src/views/layouts/common/Sidebar/components/NetworkDropdown.js (2)
84-85: Consider removing localStorage operationsSince we're moving to Redux for state management, continuing to use localStorage creates a dual source of truth. This could lead to synchronization issues.
Consider:
- Moving all state management to Redux
- If localStorage is needed for persistence, handle it in a single place (e.g., Redux middleware) rather than scattered throughout components
- Create an action creator that handles both Redux state and localStorage updates
Example middleware approach:
// Create a middleware to handle localStorage persistence const localStorageMiddleware = store => next => action => { const result = next(action); if (action.type === 'accessControl/addActiveNetwork') { localStorage.setItem('activeNetwork', JSON.stringify(action.payload)); localStorage.setItem('currentUserRole', JSON.stringify(action.payload.role)); } return result; };Also applies to: 119-120
Line range hint
89-100: Enhance error handling in loadNetworkDataThe current error handling only logs to console. Consider adding proper error handling and user feedback.
Suggested improvement:
const loadNetworkData = async (network) => { try { await Promise.all([ dispatch(loadDevicesData(network.net_name)), dispatch(loadSitesData(network.net_name)), dispatch(fetchNetworkUsers(network._id)), dispatch(loadUserRoles(network._id)), dispatch(fetchAvailableNetworkUsers(network._id)) ]); } catch (error) { - console.error('Error loading network data:', error); + console.error('Failed to load network data:', error); + // Dispatch an action to show error in UI + dispatch(setNetworkError('Failed to load network data. Please try again.')); + // Optionally revert to previous network + if (activeNetwork) { + dispatch(addActiveNetwork(activeNetwork)); + } } };netmanager/src/views/components/DataDisplay/DeviceManagement/ManagementStats.js (2)
220-221: Improve JSX formatting consistency.The JSX formatting changes improve readability, but there are some inconsistencies in indentation.
Consider using a consistent indentation style:
- }} - > + }}>Also applies to: 280-281, 292-293, 298-299, 318-319
318-319: Consider adding role and keyboard interaction for accessibility.The clickable div should have proper accessibility attributes for better user experience.
- onClick={() => history.push(`/device/${device_name}/overview`)} + onClick={() => history.push(`/device/${device_name}/overview`)} + role="button" + tabIndex={0} + onKeyPress={(e) => { + if (e.key === 'Enter') { + history.push(`/device/${device_name}/overview`); + } + }}netmanager/src/views/layouts/common/Topbar.js (1)
288-288: Enhance null safety with optional chainingConsider using optional chaining to make the code more concise and safer.
-(activeNetwork && activeNetwork.net_name) || '' +activeNetwork?.net_name || ''🧰 Tools
🪛 Biome (1.9.4)
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/views/pages/UserList/components/UsersTable/UsersTable.js (2)
326-327: Enhance dialog accessibility implementationWhile adding aria-labelledby is a good accessibility practice, it needs a corresponding element with a matching ID.
Consider this improvement:
<Dialog open={showMoreDetailsPopup} onClose={hideMoreDetailsDialog} aria-labelledby="form-dialog-title" > - <DialogTitle>User request details</DialogTitle> + <DialogTitle id="form-dialog-title">User request details</DialogTitle>Also applies to: 498-499
Line range hint
191-204: Improve error handling in deleteUser functionThe error handling could be enhanced to provide better user feedback instead of just console logging.
Consider this improvement:
const deleteUser = async () => { setIsLoading(true); dispatch(loadStatus(true)); try { await props.mappedConfirmDeleteUser(userDelState.user); hideDeleteDialog(); await dispatch(fetchNetworkUsers(activeNetwork._id)); + dispatch(updateMainAlert({ + message: 'User successfully deleted', + show: true, + severity: 'success' + })); } catch (error) { - console.error(error); + dispatch(updateMainAlert({ + message: error.response?.data?.message || 'Failed to delete user', + show: true, + severity: 'error' + })); } finally { setIsLoading(false); dispatch(loadStatus(false)); } };netmanager/src/views/pages/Roles/components/RolesTable.js (2)
Line range hint
108-164: Consider enhancing state management and error handling.While the implementation is functional, here are some suggestions to improve maintainability:
- Move loading state to Redux for consistent state management
- Centralize error messages for better maintenance
- Extract permission validation logic to a utility function
Example refactor for permission validation:
+ const validatePermissions = (permissions) => { + if (!permissions || permissions.length === 0) { + return { + isValid: false, + message: 'Please select at least one permission' + }; + } + return { isValid: true }; + }; const submitEditRole = (e) => { e.preventDefault(); setLoading(true); if (!isEmpty(updatedRole)) { const permissionIds = selectedPermissions ? selectedPermissions.map((permission) => permission.value) : []; - if (permissionIds.length > 0) { + const validationResult = validatePermissions(permissionIds); + if (validationResult.isValid) { updatePermissionsToRoleApi(updatedRole._id, { permission_ids: permissionIds })
Line range hint
214-234: Consider extracting styles and dialog components.The UI implementation is solid, but could benefit from some structural improvements:
- Move inline styles to makeStyles:
const useStyles = makeStyles((theme) => ({ // ... existing styles + actionContainer: { + display: 'flex', + flexDirection: 'column', + maxWidth: '90px' + }, + deleteButton: { + color: 'red' + } })); // Then in JSX: -<div style={{ display: 'flex', flexDirection: 'column', maxWidth: '90px' }}> +<div className={classes.actionContainer}>
- Consider extracting the edit dialog into a separate component for better maintainability:
const EditRoleDialog = ({ open, onClose, role, onSubmit }) => { // Dialog implementation };netmanager/src/views/layouts/Main.js (1)
69-117: Consider moving 'fetchUserDetails' outside of useEffect for clarityDefining
fetchUserDetailsinside theuseEffecthook can make the code harder to read and might hinder reuse. Consider moving the function definition outside ofuseEffect.Apply this diff to refactor the code:
+ const fetchUserDetails = async () => { + // fetching logic + }; useEffect(() => { // existing code - const fetchUserDetails = async () => { - // fetching logic - }; try { - fetchUserDetails(); + await fetchUserDetails(); } catch (err) { console.error(err); } finally { setLoading(false); } }, []);netmanager/src/views/components/DataDisplay/Devices.js (4)
273-273: Consider adding PropTypes forCreateDevice.To enhance type safety and code clarity, add
propTypesdefinitions to theCreateDevicecomponent, specifying the expected shape of thenetworkprop.Apply this diff to include
propTypes:import React, { useState, useEffect } from 'react'; +import PropTypes from 'prop-types'; import { useDispatch, useSelector } from 'react-redux'; const CreateDevice = ({ open, setOpen, network }) => { // component code... }; +CreateDevice.propTypes = { + open: PropTypes.bool.isRequired, + setOpen: PropTypes.func.isRequired, + network: PropTypes.object.isRequired, +};
492-492: Add PropTypes forSoftCreateDevice.For better type enforcement and readability, include
propTypesdefinitions for theSoftCreateDevicecomponent, specifying the expected structure of thenetworkprop.Apply this diff to include
propTypes:import React, { useState } from 'react'; +import PropTypes from 'prop-types'; import { useDispatch, useSelector } from 'react-redux'; const SoftCreateDevice = ({ open, setOpen, network }) => { // component code... }; +SoftCreateDevice.propTypes = { + open: PropTypes.bool.isRequired, + setOpen: PropTypes.func.isRequired, + network: PropTypes.object.isRequired, +};
797-801: Simplify theactiveNetworkchecks for clarity.Currently, you're checking
if (!activeNetwork) return;followed byif (!isEmpty(activeNetwork)) { ... }. Since you return ifactiveNetworkis falsy, the subsequent check might be redundant. Consider combining these conditions for cleaner code.Apply this diff to streamline the checks:
-if (!activeNetwork) return; - -if (!isEmpty(activeNetwork)) { +if (activeNetwork && !isEmpty(activeNetwork)) {
Line range hint
346-353: Enhance clarity in the error message.The error message uses "org" as an abbreviation. For professionalism and clarity, consider spelling out "organization".
Apply this diff to update the message:
dispatch( updateMainAlert({ - message: `You are not a member of the ${newDevice.network} organisation. Only members of the org can add devices to it. Contact support if you think this is a mistake.`, + message: `You are not a member of the ${newDevice.network} organization. Only members of the organization can add devices to it. Contact support if you think this is a mistake.`, show: true, severity: 'error' }) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
netmanager/src/views/pages/Dashboard/components/Map/Shapefiles.zipis excluded by!**/*.zip
📒 Files selected for processing (82)
netmanager/src/AppRoutes.js(0 hunks)netmanager/src/__tests__/OverlayMap/HeatMapOverlay.test.js(0 hunks)netmanager/src/__tests__/OverlayMap/OverlayMap.test.js(0 hunks)netmanager/src/config/utils.js(1 hunks)netmanager/src/views/components/Activities/ActivitiesTable.js(2 hunks)netmanager/src/views/components/AirQlouds/AirQloudRegistry.js(0 hunks)netmanager/src/views/components/AirQlouds/AirQloudToolbar.js(0 hunks)netmanager/src/views/components/AirQlouds/AirQloudsTable.js(0 hunks)netmanager/src/views/components/AirQlouds/index.js(0 hunks)netmanager/src/views/components/DataDisplay/DeviceManagement/AirqloudUptimeChart.js(4 hunks)netmanager/src/views/components/DataDisplay/DeviceManagement/ManagementStats.js(6 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DeviceDeployStatus.js(2 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DeviceEdit.js(3 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DeviceView.js(1 hunks)netmanager/src/views/components/DataDisplay/Devices.js(8 hunks)netmanager/src/views/components/Grid/GridReportsView.js(18 hunks)netmanager/src/views/components/Hosts/HostView.js(14 hunks)netmanager/src/views/components/Hosts/HostsTable.js(6 hunks)netmanager/src/views/components/SIM/SimRegistry.js(7 hunks)netmanager/src/views/components/Sites/SiteRegistry.js(2 hunks)netmanager/src/views/components/Sites/SiteToolbar.js(9 hunks)netmanager/src/views/components/Sites/SiteView.js(3 hunks)netmanager/src/views/components/Sites/SitesTable.js(1 hunks)netmanager/src/views/components/Table/UserList/UserList.js(1 hunks)netmanager/src/views/components/Table/UserList/components/UsersTable/UsersTable.js(1 hunks)netmanager/src/views/containers/PageAccess.js(1 hunks)netmanager/src/views/layouts/Main.js(2 hunks)netmanager/src/views/layouts/common/Sidebar/Sidebar.js(2 hunks)netmanager/src/views/layouts/common/Sidebar/components/NetworkDropdown.js(3 hunks)netmanager/src/views/layouts/common/Sidebar/components/SidebarNav/SidebarNav.js(3 hunks)netmanager/src/views/layouts/common/Topbar.js(1 hunks)netmanager/src/views/pages/Analytics/components/CohortsDashboard.jsx(1 hunks)netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx(1 hunks)netmanager/src/views/pages/Analytics/index.js(0 hunks)netmanager/src/views/pages/CohortsRegistry/AddCohortForm.jsx(1 hunks)netmanager/src/views/pages/CohortsRegistry/CohortDetails.jsx(18 hunks)netmanager/src/views/pages/CohortsRegistry/CohortsTable.js(1 hunks)netmanager/src/views/pages/CohortsRegistry/index.js(3 hunks)netmanager/src/views/pages/Dashboard/Dashboard.js(0 hunks)netmanager/src/views/pages/Dashboard/components/CustomDisplayChart/CustomDisplayChart.js(0 hunks)netmanager/src/views/pages/Dashboard/components/CustomDisplayChart/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/CustomisableChart/CustomisableChart.js(0 hunks)netmanager/src/views/pages/Dashboard/components/CustomisableChart/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/ExceedancesChart/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/Map/Map.js(0 hunks)netmanager/src/views/pages/Dashboard/components/Map/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/Pm25Levels/Pm25Levels.js(0 hunks)netmanager/src/views/pages/Dashboard/components/Pm25Levels/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/PollutantCategory/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/TotalProfit/TotalProfit.js(0 hunks)netmanager/src/views/pages/Dashboard/components/TotalProfit/index.js(0 hunks)netmanager/src/views/pages/Dashboard/components/index.js(0 hunks)netmanager/src/views/pages/Dashboard/index.js(0 hunks)netmanager/src/views/pages/ExportData/index.js(3 hunks)netmanager/src/views/pages/GridsRegistry/AddGridForm.jsx(4 hunks)netmanager/src/views/pages/GridsRegistry/GridsDetails.js(10 hunks)netmanager/src/views/pages/GridsRegistry/GridsTable.jsx(1 hunks)netmanager/src/views/pages/GridsRegistry/index.js(3 hunks)netmanager/src/views/pages/Heatmap/HeatMapOverlay.js(0 hunks)netmanager/src/views/pages/Map/OverlayMap.js(7 hunks)netmanager/src/views/pages/Map/components/Filter.jsx(7 hunks)netmanager/src/views/pages/Map/components/Indicator.js(4 hunks)netmanager/src/views/pages/ReportTemplate/Main.js(0 hunks)netmanager/src/views/pages/ReportTemplate/MainTest.js(0 hunks)netmanager/src/views/pages/ReportTemplate/ReportTemplate.js(0 hunks)netmanager/src/views/pages/ReportTemplate/index.js(0 hunks)netmanager/src/views/pages/Reports/Reports.js(0 hunks)netmanager/src/views/pages/Reports/graph.js(0 hunks)netmanager/src/views/pages/Reports/index.js(0 hunks)netmanager/src/views/pages/Roles/components/RolesTable.js(2 hunks)netmanager/src/views/pages/Roles/components/RolesToolbar.js(2 hunks)netmanager/src/views/pages/Roles/index.js(1 hunks)netmanager/src/views/pages/SignIn/SignIn.js(0 hunks)netmanager/src/views/pages/SignIn/index.js(0 hunks)netmanager/src/views/pages/SignUp/FunctionalLogin.js(0 hunks)netmanager/src/views/pages/SignUp/SignUp.js(0 hunks)netmanager/src/views/pages/SignUp/index.js(1 hunks)netmanager/src/views/pages/UserList/AvailableUserList.js(2 hunks)netmanager/src/views/pages/UserList/UserList.js(1 hunks)netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js(3 hunks)netmanager/src/views/pages/UserList/components/UsersTable/UsersTable.js(4 hunks)netmanager/src/views/pages/UserList/components/UsersToolbar/UsersToolbar.js(2 hunks)
⛔ Files not processed due to max files limit (1)
- netmanager/src/views/stateConnectors/usersStateConnector.js
💤 Files with no reviewable changes (35)
- netmanager/src/views/pages/Dashboard/components/Pm25Levels/index.js
- netmanager/src/views/pages/Dashboard/components/Map/index.js
- netmanager/src/views/pages/ReportTemplate/index.js
- netmanager/src/views/pages/SignIn/index.js
- netmanager/src/views/pages/Dashboard/components/ExceedancesChart/index.js
- netmanager/src/views/pages/Dashboard/index.js
- netmanager/src/views/pages/Reports/index.js
- netmanager/src/views/pages/Dashboard/components/TotalProfit/index.js
- netmanager/src/views/pages/Dashboard/components/CustomisableChart/index.js
- netmanager/src/views/pages/Reports/Reports.js
- netmanager/src/views/pages/Dashboard/components/CustomDisplayChart/index.js
- netmanager/src/views/pages/Dashboard/components/PollutantCategory/index.js
- netmanager/src/views/pages/Dashboard/components/CustomDisplayChart/CustomDisplayChart.js
- netmanager/src/views/components/AirQlouds/index.js
- netmanager/src/views/components/AirQlouds/AirQloudRegistry.js
- netmanager/src/tests/OverlayMap/OverlayMap.test.js
- netmanager/src/views/pages/Dashboard/components/TotalProfit/TotalProfit.js
- netmanager/src/views/pages/SignUp/SignUp.js
- netmanager/src/views/pages/ReportTemplate/MainTest.js
- netmanager/src/views/pages/Analytics/index.js
- netmanager/src/AppRoutes.js
- netmanager/src/views/components/AirQlouds/AirQloudToolbar.js
- netmanager/src/views/pages/Dashboard/Dashboard.js
- netmanager/src/views/components/AirQlouds/AirQloudsTable.js
- netmanager/src/views/pages/ReportTemplate/ReportTemplate.js
- netmanager/src/views/pages/Dashboard/components/index.js
- netmanager/src/views/pages/Heatmap/HeatMapOverlay.js
- netmanager/src/tests/OverlayMap/HeatMapOverlay.test.js
- netmanager/src/views/pages/SignIn/SignIn.js
- netmanager/src/views/pages/Dashboard/components/Map/Map.js
- netmanager/src/views/pages/SignUp/FunctionalLogin.js
- netmanager/src/views/pages/Dashboard/components/Pm25Levels/Pm25Levels.js
- netmanager/src/views/pages/Dashboard/components/CustomisableChart/CustomisableChart.js
- netmanager/src/views/pages/ReportTemplate/Main.js
- netmanager/src/views/pages/Reports/graph.js
✅ Files skipped from review due to trivial changes (2)
- netmanager/src/config/utils.js
- netmanager/src/views/pages/SignUp/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
netmanager/src/views/layouts/common/Topbar.js
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/views/pages/Roles/index.js
[error] 50-50: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/views/pages/UserList/AvailableUserList.js
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (45)
netmanager/src/views/pages/Map/components/Indicator.js (2)
19-20: Clean formatting improvements in the Tooltip component! 👍
The formatting changes enhance code readability while maintaining the component's functionality. The style object formatting is consistent and well-structured.
Also applies to: 37-38, 49-50, 56-57, 64-65
84-85: Well-structured air quality indicators with clear descriptions! 🌟
The formatting changes maintain consistency across all indicator tooltips while providing clear and informative descriptions for each air quality level.
Also applies to: 91-92, 98-99, 105-106, 112-113, 119-120
netmanager/src/views/pages/Map/OverlayMap.js (2)
6-8: LGTM! Import paths have been updated correctly.
The relative import paths for Filter, Indicator, and MapPopup components align with the component structure mentioned in the PR objectives.
190-191: LGTM! RichTooltip onClose handlers are properly formatted.
The self-closing tags and arrow function syntax are consistent with modern React practices.
Also applies to: 338-339
netmanager/src/views/components/SIM/SimRegistry.js (2)
12-12: Clean Redux integration implemented correctly!
The migration from localStorage to Redux's useSelector for managing activeNetwork state is well-implemented. This change improves state management consistency and follows Redux best practices.
Also applies to: 218-218
149-150: Style improvements enhance dialog component maintainability
The dialog styling improvements and click handler formatting changes are good refinements that improve code readability while maintaining functionality.
Also applies to: 166-171
netmanager/src/views/pages/Analytics/components/CohortsDashboard.jsx (1)
16-19: LGTM! Verify component file locations.
The direct imports improve code organization and make dependencies more explicit. This aligns well with the PR's cleanup objectives.
Let's verify that all imported components exist in the expected locations:
✅ Verification successful
All component imports are correctly located ✨
All imported components exist in the expected directory netmanager/src/views/pages/Analytics/components/, confirming that the import statements are valid and properly organized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and location of imported components
# Expected: All component files should exist in the same directory
fd -e jsx -e js "ExceedancesChart|PollutantCategory|AveragesChart|AddChart" --exec echo "Found: {}"
Length of output: 399
netmanager/src/views/components/DataDisplay/DeviceManagement/AirqloudUptimeChart.js (2)
47-48: LGTM: Clean implementation of guard clause
The early return when no active network exists is a good practice. It prevents unnecessary operations and potential errors.
229-230: LGTM: Minor formatting improvements
These formatting changes improve code consistency without affecting functionality.
Also applies to: 239-240
netmanager/src/views/pages/UserList/components/UsersTable/AvailableUsersTable.js (1)
128-129: LGTM! Clean formatting.
The formatting change improves code readability while maintaining functionality.
netmanager/src/views/pages/UserList/components/UsersToolbar/UsersToolbar.js (1)
447-448: LGTM - Minor formatting improvement
The Submit button formatting changes improve code readability while maintaining functionality.
netmanager/src/views/components/Hosts/HostView.js (1)
666-668: 🛠️ Refactor suggestion
Add dependency to useEffect for activeNetwork changes
The current implementation might miss updates if activeNetwork becomes available after the initial render.
- }, []);
+ }, [activeNetwork, sites]);Likely invalid or redundant comment.
netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx (1)
15-18: Verify local component imports exist
The restructuring of imports to use local components improves modularity and makes dependencies more explicit. However, let's ensure these local components exist in the same directory.
✅ Verification successful
All local component imports verified successfully
All imported components exist in the same directory as expected:
ExceedancesChart.jsPollutantCategory.jsAveragesChart.jsAddChart.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of imported component files
# Check if the imported components exist in the same directory
fd --type f --extension jsx --extension js "^(ExceedancesChart|PollutantCategory|AveragesChart|AddChart)\." $(dirname "netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx")
Length of output: 497
netmanager/src/views/components/Table/UserList/UserList.js (1)
26-30: Verify activeNetwork initialization
Let's ensure that activeNetwork is properly initialized in the Redux store, as this component now depends on it.
✅ Verification successful
Let me gather more information about the initialization of activeNetwork by checking the Redux actions and reducers.
activeNetwork initialization is properly handled
The activeNetwork state is well-managed through multiple initialization paths:
- Primary initialization in
Main.jsfrom localStorage - Network switching via
NetworkDropdowncomponent - Fallback initialization for AirQo network
- Proper persistence to localStorage for state recovery
The component's dependency on activeNetwork is safe and follows established patterns across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Redux reducer and initialization logic for activeNetwork
ast-grep --pattern 'case $action: {
$$$
activeNetwork: $_
$$$
}'
# Search for potential initialization points
rg -A 5 'activeNetwork.*='
Length of output: 35920
Script:
#!/bin/bash
# Search for Redux actions related to activeNetwork initialization
rg -A 5 'addActiveNetwork|setActiveNetwork|initializeActiveNetwork'
# Search for Redux reducer cases handling activeNetwork
ast-grep --pattern 'case $action: {
$$$
return {
$$$
activeNetwork: $_
$$$
}
}'
# Check for localStorage initialization
rg -A 5 'localStorage.setItem.*activeNetwork'
Length of output: 8262
netmanager/src/views/pages/UserList/AvailableUserList.js (2)
7-7: LGTM! Good move to Redux hooks
The addition of Redux hooks aligns well with the PR's objective of centralizing state management.
27-27: Verify Redux store initialization
The Redux selector looks good, but let's ensure the Redux store is properly initialized with the activeNetwork state.
✅ Verification successful
Let me search for the specific Redux slice and reducer for activeNetwork to complete the verification.
Let me fix the regex and check the root reducer configuration.
Redux store setup is properly configured
The activeNetwork state is correctly managed in the Redux store:
- The
accessControlReduceris properly integrated into the root reducer - It maintains the
activeNetworkstate with a dedicated actionLOAD_CURRENT_NETWORK_SUCCESS - The selector usage in the component matches the store structure:
state.accessControl.activeNetwork
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Redux store is properly configured
rg -A 5 "configureStore|createStore|activeNetwork.*reducer"
Length of output: 12730
Script:
#!/bin/bash
# Search for activeNetwork reducer and related Redux slice
rg -A 5 "accessControl.*reducer|activeNetwork.*slice"
# Also check for the root reducer configuration
rg -A 5 "rootReducer.*=|combineReducers.*{" netmanager/src/redux/
Length of output: 779
Script:
#!/bin/bash
# Check root reducer configuration and accessControl reducer
rg -A 5 "rootReducer.*=|combineReducers" netmanager/src/redux/
# Also check the accessControl reducer implementation
cat netmanager/src/redux/AccessControl/reducers.js
Length of output: 7585
netmanager/src/views/pages/CohortsRegistry/index.js (2)
14-14: Nice addition of permission control! 👍
The implementation of withPermission HOC provides appropriate access control for cohort management operations.
Also applies to: 120-120
14-14: Verify consistent permission and state management patterns
Let's verify that similar components follow the same patterns for permissions and network state management.
Also applies to: 44-44
✅ Verification successful
Let me gather more information about the permission usage to verify the consistency.
Permission and state management patterns are consistent
The codebase shows consistent patterns:
- All related components (GridsRegistry, GridsDetails, CohortsRegistry) use the same
CREATE_UPDATE_AND_DELETE_AIRQLOUDSpermission withwithPermissionHOC - The
activeNetworkselector follows a uniform pattern across all components, consistently accessingstate.accessControl.activeNetwork
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent permission usage
echo "Checking permission usage patterns..."
rg -l "CREATE_UPDATE_AND_DELETE_AIRQLOUDS" --type js
# Check for consistent activeNetwork selector usage
echo "Checking activeNetwork selector patterns..."
rg "useSelector.*activeNetwork" --type js
Length of output: 6604
Script:
#!/bin/bash
# Check how withPermission is used in the files that have CREATE_UPDATE_AND_DELETE_AIRQLOUDS
echo "Checking withPermission usage in relevant files..."
rg "withPermission" -A 3 netmanager/src/views/pages/GridsRegistry/index.js netmanager/src/views/pages/GridsRegistry/GridsDetails.js netmanager/src/views/pages/CohortsRegistry/index.js
# Check for other permission constants used with these components
echo "Checking other permission constants..."
rg "permission: ['\"].*['\"]" netmanager/src/views/pages/GridsRegistry/index.js netmanager/src/views/pages/GridsRegistry/GridsDetails.js netmanager/src/views/pages/CohortsRegistry/index.js
Length of output: 2220
netmanager/src/views/pages/Roles/components/RolesToolbar.js (1)
3-3: LGTM! Good move towards centralized state management.
The addition of useSelector aligns well with the PR's objective of standardizing state management through Redux hooks.
netmanager/src/views/pages/CohortsRegistry/AddCohortForm.jsx (2)
15-15: LGTM! Clean Redux hooks integration.
The addition of Redux hooks aligns well with the PR's objective of improving state management.
21-21: Verify Redux state structure exists.
The Redux selector looks good, but let's verify the state path exists to prevent runtime errors.
✅ Verification successful
The search results show extensive usage of state.accessControl.activeNetwork across multiple components and the presence of the accessControlReducer in the Redux store configuration. Let me verify the reducer implementation to ensure it properly manages this state.
Let me verify the reducer's handling of the activeNetwork state.
Let me try one final search to locate the action types and reducer handling.
Redux state path is properly implemented and actively used
The Redux state path state.accessControl.activeNetwork is well-defined in the codebase:
- Initialized in the
initialStateofaccessControlReducer - Updated through the
LOAD_CURRENT_NETWORK_SUCCESSaction - Widely and consistently used across multiple components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Redux state structure for activeNetwork
# Check for the Redux state structure definition
rg -A 5 "state.accessControl.activeNetwork" --type js
# Check for the Redux reducer
rg -A 10 "accessControl.*reducer" --type js
Length of output: 25884
Script:
#!/bin/bash
# Check the accessControl reducer implementation
ast-grep --pattern 'const initialState = {
$$$
activeNetwork: $_,
$$$
}'
# Backup search for reducer implementation
rg -A 10 "initialState.*=.*{" netmanager/src/redux/AccessControl/
Length of output: 945
Script:
#!/bin/bash
# Check for reducer cases handling activeNetwork
rg -A 5 "case.*SET.*NETWORK" netmanager/src/redux/AccessControl/
# Also check for action creators
rg -A 3 "export.*setActiveNetwork" netmanager/src/redux/AccessControl/
Length of output: 139
Script:
#!/bin/bash
# Search for action types related to network
rg "SET.*NETWORK.*=" netmanager/src/redux/AccessControl/types.js
# Search for reducer cases with different pattern
rg -A 5 "case.*NETWORK" netmanager/src/redux/AccessControl/reducers.js
# Search for the complete reducer file content
cat netmanager/src/redux/AccessControl/reducers.js
Length of output: 2191
netmanager/src/views/pages/Map/components/Filter.jsx (2)
86-87: LGTM! Good consistency improvement.
The data-for attribute case consistency change aligns well with the other tooltip identifiers in the component.
31-32: Consider verifying color contrast ratios for accessibility.
The explicit color settings (#29323d and #ffffff) in the control indicators should be verified for proper contrast ratios to ensure accessibility compliance, especially for the white text on colored backgrounds.
Also applies to: 45-46, 59-60, 73-74
✅ Verification successful
The previous search revealed theme files but had a regex error. Let's examine the theme configuration and color definitions more precisely to verify the contrast considerations.
The color #29323d appears to be a safe choice for contrast
The color #29323d is a dark slate gray that provides good contrast against light backgrounds. The codebase follows Material-UI's theming system which already implements WCAG contrast guidelines, and the color choice aligns well with the existing palette's text colors (blueGrey[900] and blueGrey[600]) used throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing color contrast or accessibility testing setup
rg -g '*.{js,jsx,ts,tsx}' -i '(accessibility|a11y|contrast|wcag)'
# Look for any existing theme or color constant definitions
rg -g '*.{js,jsx,ts,tsx}' -i '(theme|colors|palette).*=.*{.*#[0-9a-f]{6}'
Length of output: 3157
Script:
#!/bin/bash
# Let's search for theme and color definitions with a corrected pattern
rg -g '*.{js,jsx,ts,tsx}' 'const.*(?:theme|colors|palette).*=.*#'
# Also search specifically in the theme files we found
cat netmanager/src/theme/palette.js
Length of output: 1718
netmanager/src/views/components/Sites/SiteToolbar.js (1)
101-102: Great accessibility improvement!
Adding ARIA attributes enhances screen reader support. Well done! 👍
netmanager/src/views/pages/CohortsRegistry/CohortDetails.jsx (3)
534-535: LGTM: Style improvements are consistent
The style adjustments improve readability and maintain consistency with the existing codebase.
Also applies to: 543-544, 552-553, 576-577, 601-602
Line range hint 183-202: Consider adding error handling for missing activeNetwork
The form initialization assumes activeNetwork will be available. Consider adding a fallback or error state.
Line range hint 373-393: Verify API endpoint security measures
The exposed measurement API endpoints should be properly secured. Ensure that:
- Authentication is required
- Rate limiting is in place
- Data access is properly scoped to user permissions
netmanager/src/views/pages/CohortsRegistry/CohortsTable.js (1)
5-5: LGTM! Good transition to Redux state management
The switch from localStorage to Redux's useSelector for activeNetwork state management aligns well with the PR's objectives and modern React practices.
Also applies to: 12-13
netmanager/src/views/pages/Roles/index.js (1)
30-31: LGTM! Good Redux integration
Clean implementation of Redux state management for activeNetwork.
netmanager/src/views/pages/GridsRegistry/index.js (2)
27-28: LGTM! Clean Redux integration
Good implementation of Redux state management for activeNetwork.
53-55: LGTM! Consistent JSX formatting
Good alignment of Box component properties improves code readability.
Also applies to: 78-80
netmanager/src/views/components/DataDisplay/DeviceView/DeviceView.js (1)
27-27: LGTM! Good move to Redux state management
The switch from localStorage to Redux's useSelector for activeNetwork state management is a positive change that aligns with the PR's objectives.
netmanager/src/views/layouts/common/Sidebar/components/NetworkDropdown.js (1)
75-87:
Add missing dependencies to useEffect
The useEffect hook is missing dependencies that it uses within its callback.
Add the missing dependencies:
- }, [dispatch, userNetworks]);
+ }, [dispatch, userNetworks, activeNetwork]);Likely invalid or redundant comment.
netmanager/src/views/layouts/common/Sidebar/components/SidebarNav/SidebarNav.js (1)
4-4: LGTM! Consistent state management implementation.
The changes align with the codebase-wide migration to Redux for state management. The addition of getActiveNetwork utility suggests improved state handling capabilities.
Let's verify the usage of getActiveNetwork utility:
Also applies to: 15-15, 248-248
netmanager/src/views/components/DataDisplay/DeviceManagement/ManagementStats.js (1)
3-3: LGTM! Consistent Redux integration.
The addition of useSelector and migration of activeNetwork state to Redux maintains consistency with the application-wide refactoring effort.
Also applies to: 57-57
netmanager/src/views/components/Table/UserList/components/UsersTable/UsersTable.js (2)
185-185: LGTM: Good use of Redux selector
The change from localStorage to Redux's useSelector for managing activeNetwork state aligns with modern React practices.
188-189: Well-structured effect hook
Good implementation of the guard clause before dispatching the action. This prevents unnecessary API calls when activeNetwork is not available.
netmanager/src/views/components/DataDisplay/DeviceView/DeviceEdit.js (2)
74-75: LGTM: Consistent Redux integration
Good implementation of Redux state management in the EditDeviceForm component.
477-482: Well-structured component initialization
The implementation shows good practices:
- Proper use of useSelector for state management
- Guard clause to prevent unnecessary site data loading
- Correct dependency management in useEffect
netmanager/src/views/pages/UserList/components/UsersTable/UsersTable.js (1)
101-101: LGTM: Redux selector implementation looks good!
The addition of the Redux selector for activeNetwork aligns well with the PR's objective of moving away from localStorage to Redux for state management.
netmanager/src/views/components/DataDisplay/Devices.js (5)
280-280: Confirm that network is properly initialized in state.
In newDeviceInitState and handleRegisterClose, you're setting network: network. Make sure that the network prop is always defined to avoid any unintended undefined values in your state.
Also applies to: 325-325
499-499: Ensure network prop in SoftCreateDevice is defined before accessing network.net_name.
You're accessing network.net_name in newDeviceInitState and handleRegisterClose. Confirm that the network prop is always provided and has the expected structure to avoid runtime errors.
Also applies to: 552-552
Line range hint 616-619: Update the success message for consistency.
In the success alert, you're checking newDevice.network !== network. Since network is an object and newDevice.network might be a string, ensure this comparison works as intended to display the correct message.
273-273: Verify that all usages of CreateDevice pass the network prop.
The CreateDevice component now expects a network prop. Ensure that every instance where CreateDevice is rendered includes the network prop to prevent any undefined errors.
✅ Verification successful
Let me also check if CreateDevice is imported/exported elsewhere:
And let's specifically look at the file where CreateDevice is defined:
All usages of CreateDevice component properly include the network prop
After analyzing the code, I can confirm that the only usage of the CreateDevice component is in the DevicesTable component where it's rendered with all required props:
<CreateDevice open={registerOpen} setOpen={setRegisterOpen} network={activeNetwork} />The network prop is properly passed using the activeNetwork value from the Redux store. The component is only used once in the codebase, and this usage correctly includes all required props: open, setOpen, and network.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances of CreateDevice component usage without the 'network' prop.
# Search for CreateDevice usages without 'network' prop
ast-grep --pattern 'CreateDevice({$$$})' --lang jsx | rg -v 'network='
Length of output: 70
Script:
#!/bin/bash
# Let's try a different approach to find CreateDevice usages
# First, find all JSX files that might use CreateDevice
rg -l "CreateDevice" --type jsx
# Then look for actual usage patterns with context
rg "<CreateDevice" -A 2 --type jsx
Length of output: 124
Script:
#!/bin/bash
# Find any imports or exports of CreateDevice
rg "CreateDevice" -g "*.{js,jsx}" -A 1 -B 1
Length of output: 2863
Script:
#!/bin/bash
# Get content of the Devices.js file to see how CreateDevice is used within
cat netmanager/src/views/components/DataDisplay/Devices.js
Length of output: 27187
492-492: Verify that all usages of SoftCreateDevice pass the network prop.
The SoftCreateDevice component now expects a network prop. Ensure that all instances where SoftCreateDevice is used include the network prop to prevent potential errors.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the SoftCreateDevice component usage:
The network prop is correctly passed in all usages of SoftCreateDevice
The component definition shows that network is a required prop in its parameters ({ open, setOpen, network }), and the only usage found in the codebase (in Devices.js) correctly passes the network prop:
<SoftCreateDevice
open={softRegisterOpen}
setOpen={setSoftRegisterOpen}
network={network}
/>Additionally, the component internally uses this prop to set the default network value in the form:
network: network.net_name🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances of SoftCreateDevice component usage without the 'network' prop.
# Search for SoftCreateDevice usages without 'network' prop
ast-grep --pattern 'SoftCreateDevice({$$$})' --lang jsx | rg -v 'network='
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach to find SoftCreateDevice usages
# First, find all JSX/JS files
fd -e jsx -e js | while read -r file; do
echo "=== $file ==="
rg -A 2 "SoftCreateDevice" "$file" || true
done
Length of output: 130930
Script:
#!/bin/bash
# Also search for the component definition to understand its props
ast-grep --pattern 'const SoftCreateDevice = $_' --lang jsx
Length of output: 25591
Script:
#!/bin/bash
# Search for any imports of SoftCreateDevice
rg "import.*SoftCreateDevice"
Length of output: 31
| const AddGridToolbar = ({ open, handleClose }) => { | ||
| const dispatch = useDispatch(); | ||
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork') || {}); | ||
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); |
There was a problem hiding this comment.
💡 Codebase verification
Migration to Redux selector needs cleanup of localStorage usage
There are still several files using localStorage for activeNetwork management which should be migrated to the Redux selector for consistency:
netmanager/src/redux/Join/actions.js: Uses localStorage get/removenetmanager/src/views/layouts/Main.js: Uses localStorage get/setnetmanager/src/views/layouts/common/Sidebar/components/NetworkDropdown.js: Uses localStorage set
The Redux selector state.accessControl.activeNetwork is widely used across the codebase (40+ locations), showing good adoption. However, the remaining localStorage usage should be addressed to complete the migration and maintain consistent state management.
🔗 Analysis chain
Good migration to Redux state management.
The switch from localStorage to Redux selector aligns well with the PR's state management improvements.
Let's verify consistent usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining localStorage.activeNetwork usage
rg "localStorage.*activeNetwork"
# Check for consistent usage of the new selector
rg "useSelector.*state\.accessControl\.activeNetwork"
Length of output: 7038
| @@ -9,7 +9,7 @@ import { formatDateString } from 'utils/dateTime'; | |||
| import CustomMaterialTable from 'views/components/Table/CustomMaterialTable'; | |||
| import usersStateConnector from 'views/stateConnectors/usersStateConnector'; | |||
| import { isEmpty } from 'underscore'; | |||
| import { useDispatch } from 'react-redux'; | |||
| import { useDispatch, useSelector } from 'react-redux'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Additional localStorage.activeNetwork references found in the codebase 🔍
The migration to Redux seems incomplete. Found several files still using localStorage for activeNetwork:
netmanager/src/views/layouts/Main.jsnetmanager/src/views/layouts/common/Sidebar/components/NetworkDropdown.jsnetmanager/src/redux/Join/actions.js
This suggests we need a coordinated approach to migrate all activeNetwork state management to Redux to maintain consistency and prevent potential state synchronization issues.
🔗 Analysis chain
Nice work on the Redux migration! 👍
The switch from localStorage to Redux's useSelector is a solid improvement for state management. The implementation is clean and follows Redux best practices.
Let's verify that we've caught all localStorage references for activeNetwork:
Also applies to: 44-44
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining localStorage.activeNetwork references
rg "localStorage.*activeNetwork"
Length of output: 870
| @@ -449,9 +449,10 @@ const SiteView = (props) => { | |||
| const history = useHistory(); | |||
| const site = useSiteDetailsData(); | |||
| const dispatch = useDispatch(); | |||
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork')); | |||
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | |||
There was a problem hiding this comment.
Add activeNetwork to useEffect dependencies
The empty dependency array means the effect won't re-run when activeNetwork changes. This could lead to stale data if activeNetwork updates.
useEffect(() => {
if (!activeNetwork) return;
if (!isEmpty(activeNetwork)) {
dispatch(loadSiteDetails(params.id, activeNetwork.net_name));
}
- }, []);
+ }, [activeNetwork, params.id, dispatch]);Also applies to: 455-458
| @@ -50,7 +50,7 @@ | |||
| const [loading, setLoading] = useState(false); | |||
| const [siteInfo, setSiteInfo] = useState({}); | |||
| const [errors, setErrors] = useState({}); | |||
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork')); | |||
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to Redux needs attention - localStorage references found
There are still two remaining localStorage references for activeNetwork that need to be migrated to Redux:
netmanager/src/views/layouts/Main.js: UseslocalStorage.getItem('activeNetwork')netmanager/src/redux/Join/actions.js: UseslocalStorage.getItem('activeNetwork')
While most of the codebase (40+ locations) has been properly migrated to use Redux selector state.accessControl.activeNetwork, these two files still need to be updated for consistency.
🔗 Analysis chain
Verify complete migration from localStorage to Redux
Let's ensure all activeNetwork references have been migrated from localStorage to Redux.
Also applies to: 452-452
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining localStorage.getItem calls related to activeNetwork
rg "localStorage.getItem.*activeNetwork"
# Search for all useSelector calls related to activeNetwork to ensure consistent usage
rg "useSelector.*state\.accessControl\.activeNetwork"
Length of output: 6443
| @@ -137,7 +137,7 @@ const UsersToolbar = (props) => { | |||
|
|
|||
| const [open, setOpen] = useState(false); | |||
| const orgData = useOrgData(); | |||
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork') || {}); | |||
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to Redux selector needs completion 🔄
There are still two instances where localStorage.getItem('activeNetwork') is being used:
netmanager/src/views/layouts/Main.jsnetmanager/src/redux/Join/actions.js
While the component under review has correctly implemented the Redux selector, these remaining instances should be migrated to maintain consistency in state management.
🔗 Analysis chain
Good move switching to Redux selector! 👍
The change from localStorage to Redux selector aligns well with the PR's state management improvements. This ensures better state synchronization across components.
Let's verify that all activeNetwork usages have been migrated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining localStorage.getItem calls related to activeNetwork
rg "localStorage.getItem.*activeNetwork"
# Search for useSelector calls for activeNetwork to confirm consistent usage
rg "useSelector.*state.accessControl.activeNetwork"
Length of output: 6441
netmanager/src/views/layouts/Main.js
Outdated
| try { | ||
| fetchUserDetails(); | ||
| } catch (err) { | ||
| console.error(err); | ||
| } finally { | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
Correct async function invocation and error handling
The fetchUserDetails function is asynchronous, but it's invoked without await inside the try...catch block. This means that any errors thrown within fetchUserDetails won't be caught, and setLoading(false) might execute before the data fetching completes. To properly handle errors and the loading state, await the function call or handle the promise correctly.
Apply this diff to fix the issue:
try {
- fetchUserDetails();
+ await fetchUserDetails();
} catch (err) {
console.error(err);
} finally {
setLoading(false);
}📝 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.
| try { | |
| fetchUserDetails(); | |
| } catch (err) { | |
| console.error(err); | |
| } finally { | |
| setLoading(false); | |
| } | |
| try { | |
| await fetchUserDetails(); | |
| } catch (err) { | |
| console.error(err); | |
| } finally { | |
| setLoading(false); | |
| } |
netmanager/src/views/layouts/Main.js
Outdated
| const res = await getUserDetails(user._id); | ||
| dispatch(addUserNetworks(res.users[0].networks)); |
There was a problem hiding this comment.
Add null checks when accessing user data
When accessing res.users[0], there's a risk of encountering an error if res.users is undefined or an empty array. To prevent potential runtime exceptions, please add a check to ensure that res.users has at least one element before accessing res.users[0].
Apply this diff to fix the issue:
+ if (res.users && res.users.length > 0) {
dispatch(addUserNetworks(res.users[0].networks));
localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks));
dispatch(addUserGroupSummary(res.users[0].groups));
+ }Committable suggestion skipped: line range outside the PR's diff.
netmanager/src/views/layouts/Main.js
Outdated
| const airqoNetwork = res.users[0].networks.find((network) => network.net_name === 'airqo'); | ||
|
|
||
| if (!activeNetwork) { | ||
| dispatch(addActiveNetwork(airqoNetwork)); | ||
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | ||
| dispatch(addCurrentUserRole(airqoNetwork.role)); | ||
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | ||
| } |
There was a problem hiding this comment.
Handle the possibility of 'airqo' network being undefined
If the 'airqo' network isn't found in the user's networks, airqoNetwork will be undefined. This could lead to errors when dispatching actions or accessing its properties. Consider adding a check to ensure airqoNetwork is valid before using it.
Apply this diff to fix the issue:
const airqoNetwork = res.users[0].networks.find((network) => network.net_name === 'airqo');
- if (!activeNetwork) {
+ if (airqoNetwork && !activeNetwork) {
dispatch(addActiveNetwork(airqoNetwork));
localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork));
dispatch(addCurrentUserRole(airqoNetwork.role));
localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role));
}📝 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.
| const airqoNetwork = res.users[0].networks.find((network) => network.net_name === 'airqo'); | |
| if (!activeNetwork) { | |
| dispatch(addActiveNetwork(airqoNetwork)); | |
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | |
| dispatch(addCurrentUserRole(airqoNetwork.role)); | |
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | |
| } | |
| const airqoNetwork = res.users[0].networks.find((network) => network.net_name === 'airqo'); | |
| if (airqoNetwork && !activeNetwork) { | |
| dispatch(addActiveNetwork(airqoNetwork)); | |
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | |
| dispatch(addCurrentUserRole(airqoNetwork.role)); | |
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | |
| } |
| if (!isEmpty(network)) { | ||
| dispatch(loadDevicesData(network)); |
There was a problem hiding this comment.
💡 Codebase verification
The review comment is correct - loadDevicesData expects a network name (string)
Looking at the codebase usage patterns, loadDevicesData consistently receives network.net_name or activeNetwork.net_name across multiple files, not the network object itself. This is particularly evident in:
- NetworkDropdown.js:
dispatch(loadDevicesData(network.net_name)) - ExportData/index.js:
dispatch(loadDevicesData(activeNetwork.net_name)) - CohortsRegistry/index.js:
dispatch(loadDevicesData(activeNetwork.net_name))
You should modify the code to pass network.net_name instead of the network object.
🔗 Analysis chain
Verify the parameter passed to loadDevicesData.
In handleRegisterSubmit, you're dispatching loadDevicesData(network);. Ensure that loadDevicesData receives the expected parameter type. If network is an object, you might need to pass network.net_name instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definition of 'loadDevicesData' to confirm the expected parameter.
# Find the definition of 'loadDevicesData' function
ast-grep --pattern 'export const loadDevicesData = ($_param) => { $$$ }' --lang javascript
# Expectation: Determine whether 'loadDevicesData' expects a network name (string) or a network object.
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search to find the loadDevicesData definition
rg "loadDevicesData" -A 5
# Also search for any imports of loadDevicesData to understand its usage
rg "import.*loadDevicesData"
Length of output: 14468
| if (!isEmpty(network)) { | ||
| dispatch(loadDevicesData(network)); |
There was a problem hiding this comment.
💡 Codebase verification
The parameter for loadDevicesData should be network.net_name instead of network
Looking at the codebase usage patterns, loadDevicesData consistently expects a network name (string) as its parameter:
- In ExportData:
dispatch(loadDevicesData(activeNetwork.net_name)) - In CohortsRegistry:
dispatch(loadDevicesData(activeNetwork.net_name)) - In NetworkDropdown:
dispatch(loadDevicesData(network.net_name))
The current implementation passing network object is incorrect and should be changed to:
if (!isEmpty(network)) {
dispatch(loadDevicesData(network.net_name));🔗 Analysis chain
Confirm the parameter for loadDevicesData in SoftCreateDevice.
You're dispatching loadDevicesData(network); after device creation. Verify that the loadDevicesData function expects the correct parameter. If network is an object, consider passing network.net_name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the definition of 'loadDevicesData' to confirm the expected parameter.
# Find the definition of 'loadDevicesData' function
ast-grep --pattern 'export const loadDevicesData = ($_param) => { $$$ }' --lang javascript
# Expectation: Determine whether 'loadDevicesData' expects a network name (string) or a network object.
Length of output: 92
Script:
#!/bin/bash
# Let's try a broader search to find loadDevicesData definition
rg "loadDevicesData" -A 5
# Also search for any imports of loadDevicesData
rg "import.*loadDevicesData"
# Search for action creators related to devices
rg "export const.*devices?" -A 5
Length of output: 40040
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
netmanager/src/views/components/DataDisplay/DeviceView/DeviceEdit.js (1)
Line range hint
270-307: Consider cleaning up disabled network selection logic.The network selection is currently disabled (
isDisabled={true}), but the component still maintains state updates and a confirmation dialog for network changes. This creates unnecessary complexity in the codebase.Consider simplifying by:
- Removing the network change handler and confirmation dialog
- Or if network changes will be re-enabled in the future, add a TODO comment explaining why it's temporarily disabled
<OutlinedSelect label="Network" value={networkList.find((option) => option.value === editData.network)} - onChange={(selected) => { - setEditData({ - ...editData, - network: selected.value - }); - setConfirmOpen(true); - }} options={networkList} isSearchable placeholder="Network" name="network" error={!!errors.network} helperText={errors.network} required isDisabled={true} />netmanager/src/views/layouts/common/Sidebar/Sidebar.js (1)
249-255: Simplify user data validation logicThe current implementation has redundant checks that can be simplified.
Consider this more concise approach:
- const currentUser = JSON.parse(localStorage.getItem('currentUser')); - - if (isEmpty(currentUser)) { - return; - } - - if (!isEmpty(currentUser)) { + try { + const currentUser = JSON.parse(localStorage.getItem('currentUser')); + if (isEmpty(currentUser)) { + return; + } +netmanager/src/views/layouts/Main.js (1)
100-100: Remove debug console.log statementThe
console.logon line 100 seems to be a leftover debug statement and should be removed to clean up the code.Apply this diff to remove the debug statement:
- console.log('E DEY WORK');netmanager/src/views/pages/ExportData/index.js (2)
514-514: Maintain consistent naming conventionsConsider using
dataTypeinstead ofdatatypeto align with the naming conventions used elsewhere in your code.
697-712: Refactor repetitive form fields into reusable componentsNoticed that the 'Data Type' select field is repeated across multiple export forms. Extracting this into a shared component would reduce duplication and simplify future maintenance.
Here's how you might create a reusable component:
// DataTypeSelect.js import Select from 'react-select'; const DataTypeSelect = ({ dataType, setDataType, required = true }) => ( <Select fullWidth label="Data Type" className="reactSelect" name="data-type" placeholder="Data Type" value={dataType} options={dataTypeOptions} onChange={(options) => setDataType(options)} variant="outlined" margin="dense" required={required} /> ); export default DataTypeSelect;Then import and use it in your forms:
import DataTypeSelect from './DataTypeSelect'; // Inside your form component <Grid item md={6} xs={12}> <DataTypeSelect dataType={dataType} setDataType={setDataType} /> </Grid>Also applies to: 866-881, 1034-1049, 1202-1217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
netmanager/src/views/apis/deviceRegistry.js(0 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DeviceEdit.js(3 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DevicePhotos.js(2 hunks)netmanager/src/views/components/DataDisplay/DeviceView/DeviceToolBar.js(0 hunks)netmanager/src/views/layouts/Main.js(2 hunks)netmanager/src/views/layouts/common/Sidebar/Sidebar.js(3 hunks)netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx(1 hunks)netmanager/src/views/pages/ExportData/index.js(12 hunks)netmanager/src/views/pages/Map/components/Indicator.js(4 hunks)
💤 Files with no reviewable changes (2)
- netmanager/src/views/apis/deviceRegistry.js
- netmanager/src/views/components/DataDisplay/DeviceView/DeviceToolBar.js
✅ Files skipped from review due to trivial changes (2)
- netmanager/src/views/components/DataDisplay/DeviceView/DevicePhotos.js
- netmanager/src/views/pages/Analytics/components/GridsDashboard.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- netmanager/src/views/pages/Map/components/Indicator.js
🔇 Additional comments (20)
netmanager/src/views/components/DataDisplay/DeviceView/DeviceEdit.js (2)
2-2: LGTM! Clean Redux hook import.
The addition of useSelector import aligns with the migration from localStorage to Redux state management.
70-71: LGTM! Clean Redux selector implementation.
The activeNetwork selector is properly implemented and used consistently throughout the component.
netmanager/src/views/layouts/common/Sidebar/Sidebar.js (3)
23-23: LGTM: Clean Redux integration
The addition of the Redux operation import aligns well with the move towards centralized state management.
239-242: 🛠️ Refactor suggestion
Optimize localStorage access and add error handling
This implementation could benefit from error handling and optimization.
Consider implementing the previously suggested optimization:
if (!userNetworks) {
- const userNetworksStorage = JSON.parse(localStorage.getItem('userNetworks'));
- if (userNetworksStorage) {
- dispatch(addUserNetworks(userNetworksStorage));
+ try {
+ const userNetworksStorage = JSON.parse(localStorage.getItem('userNetworks'));
+ if (userNetworksStorage) {
+ dispatch(addUserNetworks(userNetworksStorage));
+ }
+ } catch (error) {
+ console.error('Failed to parse userNetworks from localStorage:', error);
}
}321-321: Verify effect dependencies and potential race conditions
The effect dependencies look correct, but let's verify there are no race conditions between state updates.
Let's check for potential race conditions in state updates:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the component's state management and effect patterns. We'll look for the component definition and any state updates or effects that might be relevant to our analysis.
From analyzing the codebase, I can now provide a definitive response about the effect dependencies and potential race conditions.
Effect dependencies are correctly implemented, no race conditions detected
The effect in question is used to manage user access and page visibility based on role permissions. The dependencies are properly ordered:
currentRole: Used to check permissions and determine accessible pagesactiveNetwork: Used to restore user networks from storageuserNetworks: Used to determine network-specific access
The code shows a clear hierarchical flow where:
- User networks are loaded first (separate effect with
activeNetworkdependency) - Main effect uses all three dependencies to properly sync role-based access with network context
- State updates (
setUserPages,setAdminPages,setLoading) are properly contained within the effect
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other useEffect hooks or state updates that might interfere
rg -A 5 "useEffect.*currentRole|useEffect.*activeNetwork|useEffect.*userNetworks" --type js
Length of output: 93
Script:
#!/bin/bash
# Let's try a broader search for the effect and its dependencies
# First, let's look for the component definition to understand the context
ast-grep --pattern 'const Sidebar = $_'
# Then search for useEffect declarations and state updates
rg "useEffect|setState.*currentRole|setState.*activeNetwork|setState.*userNetworks" --type js -B 2 -A 5
Length of output: 81422
netmanager/src/views/layouts/Main.js (3)
94-95: Add null checks when accessing user data
When accessing res.users[0], there's a risk of encountering an error if res.users is undefined or an empty array.
Apply this diff to fix the issue:
+ if (res.users && res.users.length > 0) {
dispatch(addUserNetworks(res.users[0].networks));
localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks));
dispatch(addUserGroupSummary(res.users[0].groups));
+ }101-108: Ensure 'airqoNetwork' is defined before using it
If airqoNetwork is undefined, it could cause runtime errors when dispatching actions or accessing its properties.
Apply this diff to fix the issue:
const airqoNetwork = res.users[0].networks.find((network) => network.net_name === 'airqo');
- if (!activeNetwork) {
+ if (airqoNetwork && !activeNetwork) {
dispatch(addActiveNetwork(airqoNetwork));
localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork));
dispatch(addCurrentUserRole(airqoNetwork.role));
localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role));
}112-118: Await the asynchronous function to properly handle errors
The fetchUserDetails function is asynchronous but is invoked without await. This means any errors thrown within fetchUserDetails won't be caught, and setLoading(false) might execute before the data fetching completes.
Apply this diff to fix the issue:
try {
- fetchUserDetails();
+ await fetchUserDetails();
} catch (err) {
console.error(err);
} finally {
setLoading(false);
}netmanager/src/views/pages/ExportData/index.js (12)
2-2: Appropriate use of useSelector hook
Great job importing useSelector from 'react-redux' to access the Redux state directly.
180-184: Addition of Data Type options is well-implemented
The introduction of dataTypeOptions enhances the user's ability to select between 'Calibrated' and 'Raw' data types.
212-213: Verify date comparisons after changing initialization
Initializing startDate and endDate as empty strings instead of null is acceptable, but please ensure that all date comparisons and validations still function correctly with this change.
218-218: Add user feedback when 'activeNetwork' is missing
While the null check for activeNetwork prevents unnecessary API calls, it would improve user experience to inform users when data loading is skipped due to missing network information.
243-243: Initialization of dataType state is appropriate
Initializing dataType with a default value of 'Calibrated' ensures a smooth user experience.
278-279: Provide alert when 'activeNetwork' is undefined
Consistent with the earlier comment, consider notifying users if activeNetwork is not set, so they understand why data isn't loading.
285-286: Notify users about missing 'activeNetwork'
Again, providing user feedback when activeNetwork is missing will enhance clarity and usability.
329-330: Reset function properly clears state
The clearExportData function correctly resets startDate, endDate, and dataType to their initial values.
Also applies to: 339-339
346-347: Ensure all required fields are validated consistently
The addition of dataType to the validation in disableDownloadBtn('sites') is appropriate. This ensures users cannot proceed without selecting a data type.
Also applies to: 353-354
362-363: Consistent validation for 'devices' export
Including dataType in the validation logic for devices maintains consistency across different export types.
Also applies to: 370-371
379-380: Validation updated for 'airqlouds' export
Good to see dataType incorporated into the validation checks here as well.
Also applies to: 387-388
396-397: Validation logic updated for 'regions' export
Including dataType in the regions export validation is consistent with other export types.
Also applies to: 404-405
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | ||
|
|
||
| useEffect(() => { | ||
| if (!activeNetwork) return; | ||
| if (isEmpty(siteOptions)) { | ||
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork')); | ||
| if (!isEmpty(activeNetwork)) { | ||
| dispatch(loadSitesData(activeNetwork.net_name)); | ||
| } | ||
| dispatch(loadSitesData(activeNetwork.net_name)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing useEffect dependencies.
While the activeNetwork check is well implemented, the empty dependency array in useEffect could lead to stale closures. The effect uses activeNetwork, dispatch, and siteOptions but doesn't list them as dependencies.
Add the missing dependencies:
useEffect(() => {
if (!activeNetwork) return;
if (isEmpty(siteOptions)) {
dispatch(loadSitesData(activeNetwork.net_name));
}
- }, []);
+ }, [activeNetwork, dispatch, siteOptions]);📝 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.
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | |
| useEffect(() => { | |
| if (!activeNetwork) return; | |
| if (isEmpty(siteOptions)) { | |
| const activeNetwork = JSON.parse(localStorage.getItem('activeNetwork')); | |
| if (!isEmpty(activeNetwork)) { | |
| dispatch(loadSitesData(activeNetwork.net_name)); | |
| } | |
| dispatch(loadSitesData(activeNetwork.net_name)); | |
| } | |
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | |
| useEffect(() => { | |
| if (!activeNetwork) return; | |
| if (isEmpty(siteOptions)) { | |
| dispatch(loadSitesData(activeNetwork.net_name)); | |
| } | |
| }, [activeNetwork, dispatch, siteOptions]); |
| device: getValues(selectedDevices), | ||
| airqlouds: getValues(selectedAirqlouds), | ||
| devices: getValues(selectedDevices), | ||
| startDateTime: roundToStartOfDay(new Date(startDate).toISOString()), | ||
| endDateTime: roundToEndOfDay(new Date(endDate).toISOString()), | ||
| frequency: frequency.value, | ||
| network: activeNetwork, | ||
| datatype: dataType.value, | ||
| pollutants: getValues(pollutants), | ||
| downloadType: 'json', | ||
| frequency: frequency.value, | ||
| downloadType: fileType.value, | ||
| outputFormat: outputFormat.value, | ||
| userId: userId, | ||
| meta_data: { | ||
| sites: extractLabels(selectedSites), | ||
| airqlouds: extractLabels(selectedAirqlouds), | ||
| devices: extractLabels(selectedDevices), | ||
| regions: extractLabels(selectedRegions) | ||
| } | ||
| minimum: true | ||
| }; | ||
|
|
||
| await scheduleExportDataApi(body) | ||
| .then((resData) => { | ||
| clearExportData(); | ||
| setLoading(false); | ||
| dispatch( | ||
| updateMainAlert({ | ||
| message: 'Data export ' + resData.data.status, | ||
| show: true, | ||
| severity: 'success' | ||
| }) | ||
| ); | ||
| }) | ||
| .catch((err) => { | ||
| dispatch( | ||
| updateMainAlert({ | ||
| message: err.response.data.message, | ||
| show: true, | ||
| severity: 'error' | ||
| }) | ||
| ); | ||
| clearExportData(); | ||
| setLoading(false); | ||
| }); | ||
| downloadDataFunc(body); |
There was a problem hiding this comment.
Verify request parameters for API call
In the submitExportData function, ensure that:
- The
deviceparameter might need to bedevicesif the API expects an array. - The
networkparameter should be verified; ifactiveNetworkis an object, confirm whether the entire object or a specific property (e.g.,activeNetwork.net_nameoractiveNetwork._id) is required by the API. - Consistency in parameter naming (e.g.,
datatypevs.dataType) throughout the codebase and API.
| startDateTime: roundToStartOfDay(new Date(startDate).toISOString()), | ||
| endDateTime: roundToEndOfDay(new Date(endDate).toISOString()), | ||
| frequency: frequency.value, | ||
| network: activeNetwork, |
There was a problem hiding this comment.
Check the 'network' parameter format
Ensure that the network parameter sent to the API matches the expected format. If activeNetwork is an object, you might need to pass a specific property like activeNetwork.net_name.
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
netmanager/src/views/layouts/Main.js (3)
1-21: Consider organizing imports for better maintainabilityConsider grouping imports in the following order:
- React and core dependencies
- Material-UI components
- Redux-related imports
- Local components
- Utilities
import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import clsx from 'clsx'; +import { isEmpty } from 'underscore'; + import { makeStyles, useTheme } from '@material-ui/styles'; import { useMediaQuery } from '@material-ui/core'; - -import { Topbar, Footer } from './common'; -import { useDispatch, useSelector } from 'react-redux'; - -import Sidebar from './common/Sidebar'; -import HorizontalLoader from '../components/HorizontalLoader/HorizontalLoader'; -import { isEmpty } from 'underscore'; +import { useDispatch, useSelector } from 'react-redux'; import { getUserDetails } from '../../redux/Join/actions'; import { addActiveNetwork, @@ -17,6 +17,10 @@ addUserGroupSummary, addUserNetworks } from '../../redux/AccessControl/operations'; + +import { Topbar, Footer } from './common'; +import Sidebar from './common/Sidebar'; +import HorizontalLoader from '../components/HorizontalLoader/HorizontalLoader'; import { LargeCircularLoader } from '../components/Loader/CircularLoader'; import { updateMainAlert } from '../../redux/MainAlert/operations';
87-89: Remove redundant user checkThe check for
isEmpty(user)is redundant since it's already checked at the beginning of the effect.- if (isEmpty(user)) { - return; - } setLoading(true); const fetchUserDetails = async () => { try { const res = await getUserDetails(JSON.parse(user)._id); if (res.success) { dispatch(addUserNetworks(res.users[0].networks)); localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks)); dispatch(addUserGroupSummary(res.users[0].groups)); - if (!isEmpty(user)) { - const airqoNetwork = res.users[0].networks.find( - (network) => network.net_name === 'airqo' - ); + const airqoNetwork = res.users[0].networks.find( + (network) => network.net_name === 'airqo' + ); - if (!activeNetwork) { - dispatch(addActiveNetwork(airqoNetwork)); - localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); - dispatch(addCurrentUserRole(airqoNetwork.role)); - localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); - } + if (!activeNetwork && airqoNetwork) { + dispatch(addActiveNetwork(airqoNetwork)); + localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); + dispatch(addCurrentUserRole(airqoNetwork.role)); + localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); }Also applies to: 101-112
68-85: Consider using a separate effect for storage initializationThe initial storage check and state initialization could be moved to a separate effect for better separation of concerns.
+ useEffect(() => { + const initializeFromStorage = () => { + const activeNetworkStorage = localStorage.getItem('activeNetwork'); + const currentUserRoleStorage = localStorage.getItem('currentUserRole'); + const userNetworksStorage = localStorage.getItem('userNetworks'); + + if (activeNetworkStorage && currentUserRoleStorage && userNetworksStorage) { + if (isEmpty(activeNetwork)) { + dispatch(addActiveNetwork(JSON.parse(activeNetworkStorage))); + } + if (isEmpty(currentRole)) { + dispatch(addCurrentUserRole(currentUserRoleStorage)); + } + if (isEmpty(userNetworks)) { + dispatch(addUserNetworks(JSON.parse(userNetworksStorage))); + } + return true; + } + return false; + }; + + return initializeFromStorage(); + }, []); + useEffect(() => { - const activeNetworkStorage = localStorage.getItem('activeNetwork'); - const currentUserRoleStorage = localStorage.getItem('currentUserRole'); - const userNetworksStorage = localStorage.getItem('userNetworks'); - - if (activeNetworkStorage && currentUserRoleStorage && userNetworksStorage) { - if (isEmpty(activeNetwork)) { - dispatch(addActiveNetwork(JSON.parse(activeNetworkStorage))); - } - if (isEmpty(currentRole)) { - dispatch(addCurrentUserRole(currentUserRoleStorage)); - } - if (isEmpty(userNetworks)) { - dispatch(addUserNetworks(JSON.parse(userNetworksStorage))); - } + const user = localStorage.getItem('currentUser'); + if (isEmpty(user)) { return; } + setLoading(true); + const fetchData = async () => { + await fetchUserDetails(); + }; + fetchData(); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
netmanager/src/views/layouts/Main.js(2 hunks)
🔇 Additional comments (2)
netmanager/src/views/layouts/Main.js (2)
61-64: LGTM! Clean Redux selector implementation
The Redux selectors are well-organized and follow best practices by selecting specific slices of state.
128-128:
Fix async function invocation
The fetchUserDetails function is called without await, which could lead to unhandled promise rejections.
- fetchUserDetails();
+ await fetchUserDetails();
netmanager/src/views/layouts/Main.js
Outdated
| const fetchUserDetails = async () => { | ||
| try { | ||
| const res = await getUserDetails(JSON.parse(user)._id); | ||
| if (res.success) { | ||
| dispatch(addUserNetworks(res.users[0].networks)); | ||
| localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks)); | ||
| dispatch(addUserGroupSummary(res.users[0].groups)); | ||
|
|
||
| if (!isEmpty(user)) { | ||
| const airqoNetwork = res.users[0].networks.find( | ||
| (network) => network.net_name === 'airqo' | ||
| ); | ||
|
|
||
| if (!activeNetwork) { | ||
| dispatch(addActiveNetwork(airqoNetwork)); | ||
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | ||
| dispatch(addCurrentUserRole(airqoNetwork.role)); | ||
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | ||
| } | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error(err); | ||
| dispatch( | ||
| updateMainAlert({ | ||
| message: err.message, | ||
| show: true, | ||
| severity: 'error' | ||
| }) | ||
| ); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add proper error handling and null checks
The fetchUserDetails function needs several improvements:
- Add null checks for
res.users[0] - Consider using try-catch for the network search
- Add type checking for the response
const fetchUserDetails = async () => {
try {
const res = await getUserDetails(JSON.parse(user)._id);
- if (res.success) {
+ if (res.success && res.users && res.users.length > 0) {
dispatch(addUserNetworks(res.users[0].networks));
localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks));
dispatch(addUserGroupSummary(res.users[0].groups));
- if (!isEmpty(user)) {
+ if (!isEmpty(user) && Array.isArray(res.users[0].networks)) {
const airqoNetwork = res.users[0].networks.find(
(network) => network.net_name === 'airqo'
);
- if (!activeNetwork) {
+ if (!activeNetwork && airqoNetwork) {
dispatch(addActiveNetwork(airqoNetwork));
localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork));
dispatch(addCurrentUserRole(airqoNetwork.role));
localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role));
}
}
}📝 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.
| const fetchUserDetails = async () => { | |
| try { | |
| const res = await getUserDetails(JSON.parse(user)._id); | |
| if (res.success) { | |
| dispatch(addUserNetworks(res.users[0].networks)); | |
| localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks)); | |
| dispatch(addUserGroupSummary(res.users[0].groups)); | |
| if (!isEmpty(user)) { | |
| const airqoNetwork = res.users[0].networks.find( | |
| (network) => network.net_name === 'airqo' | |
| ); | |
| if (!activeNetwork) { | |
| dispatch(addActiveNetwork(airqoNetwork)); | |
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | |
| dispatch(addCurrentUserRole(airqoNetwork.role)); | |
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | |
| } | |
| } | |
| } | |
| } catch (err) { | |
| console.error(err); | |
| dispatch( | |
| updateMainAlert({ | |
| message: err.message, | |
| show: true, | |
| severity: 'error' | |
| }) | |
| ); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| const fetchUserDetails = async () => { | |
| try { | |
| const res = await getUserDetails(JSON.parse(user)._id); | |
| if (res.success && res.users && res.users.length > 0) { | |
| dispatch(addUserNetworks(res.users[0].networks)); | |
| localStorage.setItem('userNetworks', JSON.stringify(res.users[0].networks)); | |
| dispatch(addUserGroupSummary(res.users[0].groups)); | |
| if (!isEmpty(user) && Array.isArray(res.users[0].networks)) { | |
| const airqoNetwork = res.users[0].networks.find( | |
| (network) => network.net_name === 'airqo' | |
| ); | |
| if (!activeNetwork && airqoNetwork) { | |
| dispatch(addActiveNetwork(airqoNetwork)); | |
| localStorage.setItem('activeNetwork', JSON.stringify(airqoNetwork)); | |
| dispatch(addCurrentUserRole(airqoNetwork.role)); | |
| localStorage.setItem('currentUserRole', JSON.stringify(airqoNetwork.role)); | |
| } | |
| } | |
| } | |
| } catch (err) { | |
| console.error(err); | |
| dispatch( | |
| updateMainAlert({ | |
| message: err.message, | |
| show: true, | |
| severity: 'error' | |
| }) | |
| ); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
netmanager/src/views/layouts/Main.js (3)
1-1: Consider grouping related imports togetherThe imports could be better organized into logical groups:
- React and core dependencies
- Material-UI components
- Redux-related imports
- Utility functions and components
import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import clsx from 'clsx'; import { makeStyles, useTheme } from '@material-ui/styles'; import { useMediaQuery } from '@material-ui/core'; +import { useDispatch, useSelector } from 'react-redux'; +import { isEmpty } from 'underscore'; import { Topbar, Footer } from './common'; -import { useDispatch, useSelector } from 'react-redux'; import Sidebar from './common/Sidebar'; import HorizontalLoader from '../components/HorizontalLoader/HorizontalLoader'; -import { isEmpty } from 'underscore'; + import { getUserDetails } from '../../redux/Join/actions'; import { addActiveNetwork,Also applies to: 8-8, 12-21
131-136: Consider adding effect cleanupThe effect updates localStorage but doesn't clean up when the component unmounts. While not critical, it's good practice to handle cleanup in effects that modify external state.
useEffect(() => { if (!isEmpty(activeNetwork)) { dispatch(addCurrentUserRole(activeNetwork.role)); localStorage.setItem('currentUserRole', JSON.stringify(activeNetwork.role)); } + return () => { + // Cleanup if component unmounts during network change + }; }, [activeNetwork]);
138-140: Consider adding specific loading messagesThe loader could provide more context about what's being loaded (network, role, or initial data) to improve user experience.
- if (!activeNetwork || loading || !currentRole) { - return <LargeCircularLoader />; + if (!activeNetwork || loading || !currentRole) { + return ( + <LargeCircularLoader + message={ + !activeNetwork ? "Loading network..." : + !currentRole ? "Loading user role..." : + "Loading user details..." + } + /> + ); }netmanager/src/views/pages/ExportData/index.js (2)
180-183: Consider using an enum or constant object for data typesThe
dataTypeOptionsarray could be better defined as an enum or constant object to ensure consistent usage throughout the codebase.-const dataTypeOptions = [ - { value: 'calibrated', label: 'Calibrated' }, - { value: 'raw', label: 'Raw' } -]; +const DATA_TYPES = { + CALIBRATED: { value: 'calibrated', label: 'Calibrated' }, + RAW: { value: 'raw', label: 'Raw' } +}; +const dataTypeOptions = Object.values(DATA_TYPES);
212-213: Initialize state consistentlyThe state initialization is inconsistent - some states use empty string, others use objects. Consider standardizing the initialization approach.
-const [startDate, setStartDate] = useState(''); -const [endDate, setEndDate] = useState(''); +const [startDate, setStartDate] = useState(null); +const [endDate, setEndDate] = useState(null); -const [dataType, setDataType] = useState({ value: 'calibrated', label: 'Calibrated' }); +const [dataType, setDataType] = useState(DATA_TYPES.CALIBRATED);Also applies to: 243-244
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
netmanager/src/views/layouts/Main.js(2 hunks)netmanager/src/views/pages/ExportData/index.js(13 hunks)
🔇 Additional comments (4)
netmanager/src/views/layouts/Main.js (2)
128-129:
Add await to async function call
The fetchUserDetails function is called without await, which means the loading state might be incorrect if an error occurs.
- fetchUserDetails();
+ await fetchUserDetails();Likely invalid or redundant comment.
61-64: Verify Redux state structure consistency
The selectors look correct, but let's verify the Redux state structure to ensure consistent access patterns across the application.
✅ Verification successful
Let me verify the Redux state structure further by checking the reducer and actions.
Let me try a different approach to find the reducer.
Let me check the AccessControl reducer specifically since we found its location.
Redux state structure is consistent and well-implemented
The selectors in Main.js perfectly align with the Redux state structure defined in the reducer. The accessControl slice maintains consistent state properties:
activeNetworkis widely used across 40+ componentscurrentRoleis used in authorization contextsuserNetworksis used for network management
All these fields are properly initialized in the initial state and managed through corresponding action handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these selectors to verify consistency
rg "state\.accessControl\.(activeNetwork|currentRole|userNetworks)" -t js
Length of output: 7234
Script:
#!/bin/bash
# Search for the accessControl reducer and its state structure
ast-grep --pattern 'const accessControlReducer = $_' -l js
rg "initialState.*=.*{" -A 10 -g "**/reducers/*access*.js"
Length of output: 221
Script:
#!/bin/bash
# Search for reducer files and state structure
fd "reducer" --type f
rg "state\.accessControl" -B 5 -g "**/reducers/*.js"
Length of output: 978
Script:
#!/bin/bash
# Check the AccessControl reducer implementation
cat netmanager/src/redux/AccessControl/reducers.js
Length of output: 1482
netmanager/src/views/pages/ExportData/index.js (2)
277-278: Add error handling for missing activeNetwork
While the null checks for activeNetwork prevent unnecessary API calls, users should be informed when data loading is skipped due to missing network information.
Also applies to: 282-283
505-513: Verify API contract for request payload
The request payload structure has been modified. Ensure these changes align with the API contract:
deviceparameter might need to be pluraldatatypevsdataTypenaming consistency- New
minimumfield addition
|
|
||
| <Grid item md={6} xs={12}> | ||
| <Select | ||
| fullWidth | ||
| label="Data Type" | ||
| className="reactSelect" | ||
| name="data-type" | ||
| placeholder="Data Type" | ||
| value={dataType} | ||
| options={dataTypeOptions} | ||
| onChange={(options) => setDataType(options)} | ||
| variant="outlined" | ||
| margin="dense" | ||
| required | ||
| /> | ||
| </Grid> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in data type selector
The data type selector component is duplicated across all four tab panels. Consider extracting it into a reusable component.
Create a new component:
const DataTypeSelector = ({ value, onChange }) => (
<Grid item md={6} xs={12}>
<Select
fullWidth
label="Data Type"
className="reactSelect"
name="data-type"
placeholder="Data Type"
value={value}
options={dataTypeOptions}
onChange={onChange}
variant="outlined"
margin="dense"
required
/>
</Grid>
);Then use it in each tab panel:
<DataTypeSelector value={dataType} onChange={(options) => setDataType(options)} />Also applies to: 861-875, 1029-1043, 1197-1211
|
New netmanager changes available for preview here |
Summary of Changes (What does this PR do?)
This pull request includes several changes focused on removing unused components and improving the use of the Redux store in the
netmanagerproject. The most important changes include the removal of theHeatMapOverlaycomponent and its associated tests, the cleanup of unused components related toAirQlouds, and the refactoring of various components to use the Redux store instead of local storage. Includes fixing data export issues.Component Removal:
netmanager/src/AppRoutes.js: Removed the import statements forAirQloudRegistry,AirQloudView, andHeatMapOverlaycomponents as they are no longer used.netmanager/src/views/components/AirQlouds/AirQloudRegistry.js: Deleted the entire file, removing theAirQloudRegistrycomponent.netmanager/src/views/components/AirQlouds/AirQloudToolbar.js: Deleted the entire file, removing theAirQloudToolbarcomponent.netmanager/src/views/components/AirQlouds/AirQloudsTable.js: Deleted the entire file, removing theAirQloudsTablecomponent.netmanager/src/__tests__/OverlayMap/HeatMapOverlay.test.js: Deleted the entire file, removing the tests for theHeatMapOverlaycomponent.Redux Store Improvements:
netmanager/src/views/components/Activities/ActivitiesTable.js: Refactored to use theuseSelectorhook to accessactiveNetworkfrom the Redux store instead of local storage.netmanager/src/views/components/DataDisplay/DeviceManagement/AirqloudUptimeChart.js: Refactored to use theuseSelectorhook to accessactiveNetworkfrom the Redux store instead of local storage.netmanager/src/views/components/DataDisplay/DeviceManagement/ManagementStats.js: Refactored to use theuseSelectorhook to access the Redux store.Code Cleanup:
netmanager/src/config/utils.js: Reformatted theURL_PATTERNregex for better readability.netmanager/src/views/apis/deviceRegistry.js: Removed the unusedEVENTSimport.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
activeNetworkacross various components, improving data consistency and reducing reliance on local storage.Bug Fixes
useEffecthooks to prevent unnecessary API calls whenactiveNetworkis not available.Deleted Components
AirQloudRegistry,SignIn, andSignUp.Dashboard,ReportTemplate, and various chart components.Refactor
useSelectorfor accessing Redux state, enhancing clarity and maintainability of the code.Style