Added user preferences page #2197
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements for managing user preferences within the application. A new route for user preferences is established in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
netmanager/src/config/urls/authService.js (1)
3-3: Consider adding JSDoc documentation.Since this is a crucial base URL constant that's now exported, it would be helpful to add JSDoc documentation explaining its purpose and usage.
+/** Base URL for authentication service v2 endpoints */ export const BASE_AUTH_SERVICE_URL_V2 = stripTrailingSlash(process.env.REACT_APP_BASE_URL_V2);netmanager/src/views/pages/Preferences/Preferences.js (4)
223-226: Enhance error feedback when removing a siteThe error message displayed is generic. Providing the site name can help users understand which removal failed.
Update the snackbar message to include the site information:
setSnackbarMessage('Error removing site'); + setSnackbarMessage(`Error removing site: ${siteToRemove.name}`);
246-268: Handle empty selected sites gracefullyIf no sites are selected, the UI might appear sparse or confusing to the user.
Display a message or placeholder when
selectedSitesis empty:{selectedSites.length === 0 ? ( <Typography variant="body1"> + You have no selected sites. Click "Add Sites" to begin. </Typography> ) : ( <Grid container spacing={2}> {selectedSites.map((site) => ( // Existing code... ))} </Grid> )}
232-234: Make the page title more descriptive"User Preferences" is a bit generic. A more specific title can enhance user understanding.
Consider changing it to "Manage Site Preferences":
<Typography variant="h4" className={classes.title}> - User Preferences + Manage Site Preferences </Typography>
198-214: Handle save errors with retries or fallbacksIf saving sites fails, users might lose their newly selected sites without an option to retry.
Implement a retry mechanism or prompt users to retry saving:
} catch (error) { console.error('Error updating sites:', error); setSnackbarMessage('Error updating sites'); setSnackbarOpen(true); + // Optional: Offer retry logic + setModalOpen(true); } finally { setIsSaving(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- netmanager/src/AppRoutes.js (2 hunks)
- netmanager/src/config/urls/authService.js (1 hunks)
- netmanager/src/views/apis/authService.js (2 hunks)
- netmanager/src/views/layouts/common/Sidebar/Sidebar.js (2 hunks)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🔇 Additional comments (6)
netmanager/src/config/urls/authService.js (1)
3-3: LGTM! Let's verify the constant's usage.The addition of the export keyword is appropriate for the new user preferences feature.
Let's verify all imports of this constant:
✅ Verification successful
Export of BASE_AUTH_SERVICE_URL_V2 is properly used and consistent with codebase patterns
The constant is:
- Used correctly in the auth service APIs, including the new user preferences endpoints
- Already referenced by multiple URI definitions in the same file
- Follows the same pattern as other service URL configurations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all files importing BASE_AUTH_SERVICE_URL_V2 # Expected: Should see imports in the new preferences-related files rg -l "import.*BASE_AUTH_SERVICE_URL_V2.*from.*authService"Length of output: 102
Script:
#!/bin/bash # Let's check the actual usage of BASE_AUTH_SERVICE_URL_V2 in the codebase rg "BASE_AUTH_SERVICE_URL_V2" -A 2 -B 2 # Also check if there are any direct references without import rg "process\.env\.REACT_APP_BASE_URL_V2" -A 2 -B 2Length of output: 7207
netmanager/src/views/apis/authService.js (2)
11-11: LGTM! Import statement follows existing patterns.
87-99: Verify integration with the Preferences component.Let's ensure these APIs are properly integrated with the new preferences page.
✅ Verification successful
Let me analyze the actual implementation and error handling before making a final determination about the integration.
APIs are properly integrated with error handling and state management ✅
The integration looks solid with:
- Proper error handling with try-catch blocks in Preferences component
- User feedback via snackbar messages for success/error states
- Loading states management during API calls
- Redux integration for state management in userDefaultsSlice
- Consistent API usage across the platform
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how these APIs are used in the Preferences component # Test: Look for imports and usage of these APIs rg -l "getUserPreferencesApi|setUserPreferencesApi" --type js # Test: Check for proper error handling in components using these APIs ast-grep --pattern 'try { $$$ getUserPreferencesApi() $$$ } catch'Length of output: 388
Script:
#!/bin/bash # Let's check the actual implementation in Preferences.js and error handling rg -A 10 "getUserPreferencesApi|setUserPreferencesApi" netmanager/src/views/pages/Preferences/Preferences.js # Also check the Redux integration rg -A 10 "getUserPreferencesApi|setUserPreferencesApi" platform/src/lib/store/services/charts/userDefaultsSlice.js platform/src/lib/store/services/account/UserDefaultsSlice.js # And check the core API wrapper rg -A 10 "getUserPreferencesApi|setUserPreferencesApi" platform/src/core/apis/Account.jsLength of output: 5959
netmanager/src/AppRoutes.js (2)
125-130: LGTM! Route implementation follows best practices.The new route:
- Is properly protected using PrivateRoute
- Uses consistent path structure with other admin routes
- Correctly implements the MainLayout
125-130: Verify route integration with navigation.Let's ensure the new route is properly integrated with the sidebar navigation.
✅ Verification successful
Route is properly integrated with navigation ✓
The verification confirms that:
- The route
/admin/users/preferencesis correctly defined inAppRoutes.js- The sidebar navigation in
netmanager/src/views/layouts/common/Sidebar/Sidebar.jsincludes the corresponding link- No route conflicts exist as this is the only occurrence of this exact path
- The route follows the existing pattern of other
/admin/users/*routes🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the preferences route is properly linked in the sidebar navigation # Test: Search for preferences route in sidebar navigation echo "Checking sidebar navigation for preferences link..." rg -i "preferences" "src/views/layouts/common/Sidebar" # Test: Check for potential route conflicts echo "Checking for route path conflicts..." rg -i "path=\"/admin/users/preferences\"" "src/" --type jsLength of output: 423
Script:
#!/bin/bash # Let's first find the correct paths using fd echo "Finding sidebar navigation files..." fd -e js -e jsx sidebar -i echo "Finding route definitions..." fd -e js -e jsx routes -i # Once we find the files, let's check their content echo "Checking for preferences route and navigation..." rg -i "preferences" --type js # Let's also check for any admin/users routes to understand the structure echo "Checking admin/users route patterns..." rg -i "path=\"/admin/users" --type jsLength of output: 19784
netmanager/src/views/layouts/common/Sidebar/Sidebar.js (1)
212-213: Verify permission requirements for the preferences page.Other sensitive sections under the Users menu (like 'Assigned Users' and 'Available Users') typically require specific permissions. Consider whether the preferences page should also be protected with appropriate permission checks.
✅ Verification successful
Based on the analysis of the codebase, I can generate the final response:
The preferences page is already protected by the PrivateRoute component
The preferences page is secured through the
PrivateRoutecomponent which checks for authentication. Unlike other sensitive sections that require specific permissions, the preferences page is intentionally designed to be accessible to all authenticated users since it manages personal user preferences like selected sites, which is a basic user-level functionality that shouldn't be restricted by role permissions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other files implement permission checks for the preferences route # Test 1: Look for route definition with permission checks rg -A 5 "path.*admin/users/preferences" # Test 2: Look for permission constants or definitions related to preferences rg -i "permission.*preference"Length of output: 444
Script:
#!/bin/bash # Let's check for permission checks in the Preferences component and related files # Check for the Preferences component implementation ast-grep --pattern 'const Preferences = $_' # Look for any permission-related hooks or checks in the codebase rg -A 3 "usePermission|hasPermission|requirePermission" # Check if PrivateRoute implements any permission checks ast-grep --pattern 'const PrivateRoute = $_' # Look for permission constants or enums rg -A 5 "PERMISSION|Permission" --type jsLength of output: 72903
| export const getUserPreferencesApi = async () => { | ||
| return await createAxiosInstance() | ||
| .get(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and moving the endpoint path to constants.
The implementation follows the existing patterns, but could be enhanced with:
- Error handling for failed requests
- Moving
/users/preferences/selected-sitesto the URL constants file - JSDoc to document the expected response structure
Example implementation:
+const PREFERENCES_ENDPOINT = '/users/preferences/selected-sites';
+/**
+ * Fetches user preferences for selected sites
+ * @returns {Promise<{selected_sites: string[]}>} The user preferences
+ * @throws {AxiosError} When the request fails
+ */
export const getUserPreferencesApi = async () => {
+ try {
return await createAxiosInstance()
- .get(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`)
+ .get(`${BASE_AUTH_SERVICE_URL_V2}${PREFERENCES_ENDPOINT}`)
.then((response) => response.data);
+ } catch (error) {
+ console.error('Failed to fetch user preferences:', error);
+ throw error;
+ }
};📝 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.
| export const getUserPreferencesApi = async () => { | |
| return await createAxiosInstance() | |
| .get(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`) | |
| .then((response) => response.data); | |
| }; | |
| const PREFERENCES_ENDPOINT = '/users/preferences/selected-sites'; | |
| /** | |
| * Fetches user preferences for selected sites | |
| * @returns {Promise<{selected_sites: string[]}>} The user preferences | |
| * @throws {AxiosError} When the request fails | |
| */ | |
| export const getUserPreferencesApi = async () => { | |
| try { | |
| return await createAxiosInstance() | |
| .get(`${BASE_AUTH_SERVICE_URL_V2}${PREFERENCES_ENDPOINT}`) | |
| .then((response) => response.data); | |
| } catch (error) { | |
| console.error('Failed to fetch user preferences:', error); | |
| throw error; | |
| } | |
| }; |
| export const setUserPreferencesApi = async (selectedSites) => { | ||
| return await createAxiosInstance() | ||
| .post(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`, { | ||
| selected_sites: selectedSites | ||
| }) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling, input validation, and documentation.
The implementation is functional but could benefit from:
- Error handling for failed requests
- Input validation for selectedSites parameter
- JSDoc documentation
- Consistent case convention in the payload
Example implementation:
+/**
+ * Updates user preferences for selected sites
+ * @param {string[]} selectedSites - Array of site IDs to save
+ * @returns {Promise<{message: string}>} Success response
+ * @throws {Error} When selectedSites is invalid
+ * @throws {AxiosError} When the request fails
+ */
export const setUserPreferencesApi = async (selectedSites) => {
+ if (!Array.isArray(selectedSites)) {
+ throw new Error('selectedSites must be an array');
+ }
+
+ try {
return await createAxiosInstance()
.post(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`, {
- selected_sites: selectedSites
+ selectedSites // API can handle camelCase to snake_case conversion
})
.then((response) => response.data);
+ } catch (error) {
+ console.error('Failed to update user preferences:', error);
+ throw error;
+ }
};📝 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.
| export const setUserPreferencesApi = async (selectedSites) => { | |
| return await createAxiosInstance() | |
| .post(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`, { | |
| selected_sites: selectedSites | |
| }) | |
| .then((response) => response.data); | |
| }; | |
| /** | |
| * Updates user preferences for selected sites | |
| * @param {string[]} selectedSites - Array of site IDs to save | |
| * @returns {Promise<{message: string}>} Success response | |
| * @throws {Error} When selectedSites is invalid | |
| * @throws {AxiosError} When the request fails | |
| */ | |
| export const setUserPreferencesApi = async (selectedSites) => { | |
| if (!Array.isArray(selectedSites)) { | |
| throw new Error('selectedSites must be an array'); | |
| } | |
| try { | |
| return await createAxiosInstance() | |
| .post(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`, { | |
| selectedSites // API can handle camelCase to snake_case conversion | |
| }) | |
| .then((response) => response.data); | |
| } catch (error) { | |
| console.error('Failed to update user preferences:', error); | |
| throw error; | |
| } | |
| }; |
netmanager/src/AppRoutes.js
Outdated
| @@ -13,6 +13,7 @@ import PermissionDenied from './views/pages/PermissionDenied'; | |||
| import { logoutUser } from './redux/Join/actions'; | |||
| import { connect } from 'react-redux'; | |||
| import ConfirmDialog from './views/containers/ConfirmDialog'; | |||
| import Preferences from './views/pages/Preferences/Preferences'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using lazy loading for the Preferences component.
The Preferences component is directly imported while most other page components are lazy-loaded. For consistency and optimal initial load time, consider converting this to a lazy import.
Apply this change:
-import Preferences from './views/pages/Preferences/Preferences';
+const Preferences = lazy(() => import('./views/pages/Preferences/Preferences'));Committable suggestion was skipped due to low confidence.
| @@ -45,6 +45,7 @@ import GroupAddIcon from '@material-ui/icons/GroupAdd'; | |||
| import AssignmentIndIcon from '@material-ui/icons/AssignmentInd'; | |||
| import AppsIcon from '@material-ui/icons/Apps'; | |||
| import DeviceHubIcon from '@material-ui/icons/DeviceHub'; | |||
| import SettingsApplicationsIcon from '@material-ui/icons/SettingsApplications'; | |||
There was a problem hiding this comment.
Remove unused import.
The SettingsApplicationsIcon is imported but not used in the component.
-import SettingsApplicationsIcon from '@material-ui/icons/SettingsApplications';📝 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.
| import SettingsApplicationsIcon from '@material-ui/icons/SettingsApplications'; |
| className={classes.autocomplete} | ||
| options={availableSites} | ||
| getOptionLabel={(option) => `${option.name} (${option.search_name})`} | ||
| value={null} | ||
| inputValue={manualSiteInput} | ||
| onInputChange={(event, newInputValue) => { | ||
| setManualSiteInput(newInputValue); | ||
| }} | ||
| onChange={handleAddManualSite} | ||
| renderInput={(params) => ( | ||
| <TextField {...params} label="Enter site name" variant="outlined" /> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Autocomplete for large datasets
The Autocomplete component may face performance issues with a large number of availableSites. Implementing virtualization can enhance user experience.
Consider using the Virtualize component from Material-UI:
import { Autocomplete, createFilterOptions } from '@material-ui/lab';
+ import VirtualizedList from '@material-ui/core/VirtualizedList';
...
<Autocomplete
className={classes.autocomplete}
options={availableSites}
+ ListboxComponent={VirtualizedList}
getOptionLabel={(option) => `${option.name} (${option.search_name})`}
...
/>Committable suggestion was skipped due to low confidence.
| setIsLoading(true); | ||
| try { | ||
| const response = await getUserPreferencesApi(); | ||
| setSelectedSites(response.selected_sites || []); |
There was a problem hiding this comment.
Check for undefined response in fetchSelectedSites
If the API doesn't return a response, accessing response.selected_sites may cause an error.
Add a check to handle undefined responses:
const fetchSelectedSites = async () => {
setIsLoading(true);
try {
const response = await getUserPreferencesApi();
+ const sites = response?.selected_sites || [];
setSelectedSites(sites);
} catch (error) {
console.error('Error fetching selected sites:', error);
setSnackbarMessage('Error fetching selected sites');
setSnackbarOpen(true);
} finally {
setIsLoading(false);
}
};Committable suggestion was skipped due to low confidence.
| useEffect(() => { | ||
| fetchSelectedSites(); | ||
| if (allSites.length === 0 && activeNetwork) { | ||
| dispatch(loadSitesSummary(activeNetwork.net_name)); | ||
| } | ||
| }, [dispatch, activeNetwork]); |
There was a problem hiding this comment.
Include allSites.length in useEffect dependencies
The useEffect hook depends on allSites.length but doesn't include it in the dependency array. This might lead to missed updates when allSites changes.
Add allSites.length to ensure the effect responds to changes in the sites data:
useEffect(() => {
fetchSelectedSites();
if (allSites.length === 0 && activeNetwork) {
dispatch(loadSitesSummary(activeNetwork.net_name));
}
- }, [dispatch, activeNetwork]);
+ }, [dispatch, activeNetwork, allSites.length]);📝 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.
| useEffect(() => { | |
| fetchSelectedSites(); | |
| if (allSites.length === 0 && activeNetwork) { | |
| dispatch(loadSitesSummary(activeNetwork.net_name)); | |
| } | |
| }, [dispatch, activeNetwork]); | |
| useEffect(() => { | |
| fetchSelectedSites(); | |
| if (allSites.length === 0 && activeNetwork) { | |
| dispatch(loadSitesSummary(activeNetwork.net_name)); | |
| } | |
| }, [dispatch, activeNetwork, allSites.length]); |
| const updatedSites = [...selectedSites, ...newSelectedSites]; | ||
| await setUserPreferencesApi(updatedSites); | ||
| setSelectedSites(updatedSites); |
There was a problem hiding this comment.
Ensure merged site list has unique entries
When combining selectedSites and newSelectedSites, duplicates may occur if the same site exists in both arrays. This can cause issues in displaying and managing user preferences.
Modify the merge to eliminate duplicates based on site_id:
const handleSaveSites = async () => {
setIsSaving(true);
try {
- const updatedSites = [...selectedSites, ...newSelectedSites];
+ const siteMap = new Map();
+ [...selectedSites, ...newSelectedSites].forEach((site) => {
+ siteMap.set(site.site_id, site);
+ });
+ const updatedSites = Array.from(siteMap.values());
await setUserPreferencesApi(updatedSites);
setSelectedSites(updatedSites);
setModalOpen(false);
setSnackbarMessage('Sites updated successfully');
setSnackbarOpen(true);
} catch (error) {
console.error('Error updating sites:', error);
setSnackbarMessage('Error updating sites');
setSnackbarOpen(true);
} finally {
setIsSaving(false);
}
};Committable suggestion was skipped due to low confidence.
| const handleAddManualSite = (event, newValue) => { | ||
| if (newValue) { | ||
| const { _id, name, search_name } = newValue; | ||
| const newSite = { | ||
| site_id: _id, | ||
| name, | ||
| search_name | ||
| }; | ||
| setNewSelectedSites([...newSelectedSites, newSite]); | ||
| setManualSiteInput(''); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Prevent duplicate sites in new selections
Currently, there's no check to prevent adding the same site multiple times to newSelectedSites. This could result in duplicate entries when saving user preferences.
Consider updating the handleAddManualSite function to prevent duplicates:
const handleAddManualSite = (event, newValue) => {
if (newValue) {
+ const isAlreadySelected = newSelectedSites.some(
+ (site) => site.site_id === newValue._id
+ );
+ if (isAlreadySelected) {
+ setManualSiteInput('');
+ return;
+ }
const { _id, name, search_name } = newValue;
const newSite = {
site_id: _id,
name,
search_name
};
setNewSelectedSites([...newSelectedSites, newSite]);
setManualSiteInput('');
}
};📝 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 handleAddManualSite = (event, newValue) => { | |
| if (newValue) { | |
| const { _id, name, search_name } = newValue; | |
| const newSite = { | |
| site_id: _id, | |
| name, | |
| search_name | |
| }; | |
| setNewSelectedSites([...newSelectedSites, newSite]); | |
| setManualSiteInput(''); | |
| } | |
| }; | |
| const handleAddManualSite = (event, newValue) => { | |
| if (newValue) { | |
| const isAlreadySelected = newSelectedSites.some( | |
| (site) => site.site_id === newValue._id | |
| ); | |
| if (isAlreadySelected) { | |
| setManualSiteInput(''); | |
| return; | |
| } | |
| const { _id, name, search_name } = newValue; | |
| const newSite = { | |
| site_id: _id, | |
| name, | |
| search_name | |
| }; | |
| setNewSelectedSites([...newSelectedSites, newSite]); | |
| setManualSiteInput(''); | |
| } | |
| }; |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🔇 Additional comments (2)
netmanager/src/views/pages/Preferences/Preferences.js (2)
1-131: Well-structured imports and styles!The component follows Material-UI best practices with comprehensive styling and proper organization of imports.
1-385: Overall well-implemented preferences pageThe component demonstrates good practices in terms of:
- Proper error handling and loading states
- Responsive design with Material-UI
- Clear separation of concerns between UI and business logic
With the suggested improvements for state management, race conditions, and accessibility, this will be a robust implementation.
| const Preferences = () => { | ||
| const classes = useStyles(); | ||
| const dispatch = useDispatch(); | ||
| const [selectedSites, setSelectedSites] = useState([]); | ||
| const [snackbarOpen, setSnackbarOpen] = useState(false); | ||
| const [snackbarMessage, setSnackbarMessage] = useState(''); | ||
| const [snackbarSeverity, setSnackbarSeverity] = useState('success'); | ||
| const [modalOpen, setModalOpen] = useState(false); | ||
| const [newSelectedSites, setNewSelectedSites] = useState([]); | ||
| const [manualSiteInput, setManualSiteInput] = useState(''); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| const [isSaving, setIsSaving] = useState(false); | ||
| const [error, setError] = useState(null); | ||
|
|
||
| const allSites = useSitesSummaryData(); | ||
| const activeNetwork = useSelector((state) => state.accessControl.activeNetwork); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing state management
The component uses multiple separate state variables for snackbar-related states. Consider combining them into a single state object for better maintainability.
- const [snackbarOpen, setSnackbarOpen] = useState(false);
- const [snackbarMessage, setSnackbarMessage] = useState('');
- const [snackbarSeverity, setSnackbarSeverity] = useState('success');
+ const [snackbar, setSnackbar] = useState({
+ open: false,
+ message: '',
+ severity: 'success'
+ });Then update the snackbar handlers accordingly:
const showSnackbar = (message, severity = 'success') => {
setSnackbar({ open: true, message, severity });
};
const hideSnackbar = () => {
setSnackbar(prev => ({ ...prev, open: false }));
};| const handleRemoveSite = async (siteToRemove) => { | ||
| setIsSaving(true); | ||
| setError(null); | ||
| try { | ||
| const updatedSites = selectedSites.filter((site) => site.site_id !== siteToRemove.site_id); | ||
| await setUserPreferencesApi(updatedSites); | ||
| setSelectedSites(updatedSites); | ||
| setSnackbarMessage('Site removed successfully'); | ||
| setSnackbarSeverity('success'); | ||
| setSnackbarOpen(true); | ||
| } catch (error) { | ||
| console.error('Error removing site:', error); | ||
| setError('Failed to remove site. Please try again later.'); | ||
| setSnackbarMessage('Error removing site'); | ||
| setSnackbarSeverity('error'); | ||
| setSnackbarOpen(true); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Prevent race conditions in site removal
The site removal process could lead to inconsistencies if multiple removal operations are triggered in quick succession.
Consider implementing optimistic updates with rollback:
const handleRemoveSite = async (siteToRemove) => {
setIsSaving(true);
setError(null);
+ const previousSites = selectedSites;
+ setSelectedSites(sites => sites.filter(site => site.site_id !== siteToRemove.site_id));
try {
- const updatedSites = selectedSites.filter((site) => site.site_id !== siteToRemove.site_id);
- await setUserPreferencesApi(updatedSites);
- setSelectedSites(updatedSites);
+ await setUserPreferencesApi(selectedSites.filter(site => site.site_id !== siteToRemove.site_id));
setSnackbarMessage('Site removed successfully');
setSnackbarSeverity('success');
setSnackbarOpen(true);
} catch (error) {
+ setSelectedSites(previousSites);
console.error('Error removing site:', error);
setError('Failed to remove site. Please try again later.');
setSnackbarMessage('Error removing site');
setSnackbarSeverity('error');
setSnackbarOpen(true);
} finally {
setIsSaving(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.
| const handleRemoveSite = async (siteToRemove) => { | |
| setIsSaving(true); | |
| setError(null); | |
| try { | |
| const updatedSites = selectedSites.filter((site) => site.site_id !== siteToRemove.site_id); | |
| await setUserPreferencesApi(updatedSites); | |
| setSelectedSites(updatedSites); | |
| setSnackbarMessage('Site removed successfully'); | |
| setSnackbarSeverity('success'); | |
| setSnackbarOpen(true); | |
| } catch (error) { | |
| console.error('Error removing site:', error); | |
| setError('Failed to remove site. Please try again later.'); | |
| setSnackbarMessage('Error removing site'); | |
| setSnackbarSeverity('error'); | |
| setSnackbarOpen(true); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; | |
| const handleRemoveSite = async (siteToRemove) => { | |
| setIsSaving(true); | |
| setError(null); | |
| const previousSites = selectedSites; | |
| setSelectedSites(sites => sites.filter(site => site.site_id !== siteToRemove.site_id)); | |
| try { | |
| await setUserPreferencesApi(selectedSites.filter(site => site.site_id !== siteToRemove.site_id)); | |
| setSnackbarMessage('Site removed successfully'); | |
| setSnackbarSeverity('success'); | |
| setSnackbarOpen(true); | |
| } catch (error) { | |
| setSelectedSites(previousSites); | |
| console.error('Error removing site:', error); | |
| setError('Failed to remove site. Please try again later.'); | |
| setSnackbarMessage('Error removing site'); | |
| setSnackbarSeverity('error'); | |
| setSnackbarOpen(true); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; |
| <Modal className={classes.modal} open={modalOpen} onClose={handleCloseModal}> | ||
| <div className={classes.modalContent}> | ||
| <Typography variant="h6" gutterBottom> | ||
| Select sites | ||
| </Typography> | ||
| <Autocomplete | ||
| className={classes.autocomplete} | ||
| options={availableSites} | ||
| getOptionLabel={(option) => `${option.name} (${option.search_name})`} | ||
| value={null} | ||
| inputValue={manualSiteInput} | ||
| onInputChange={(event, newInputValue) => { | ||
| setManualSiteInput(newInputValue); | ||
| }} | ||
| onChange={handleAddManualSite} | ||
| renderInput={(params) => ( | ||
| <TextField {...params} label="Enter site name" variant="outlined" /> | ||
| )} | ||
| /> | ||
| <div className={classes.chipContainer}> | ||
| {newSelectedSites.map((site) => ( | ||
| <Chip | ||
| key={site.site_id} | ||
| label={`${site.name} (${site.search_name})`} | ||
| onDelete={() => handleRemoveNewSite(site)} | ||
| className={classes.chip} | ||
| /> | ||
| ))} | ||
| </div> | ||
| <div className={classes.wrapper}> | ||
| <Button | ||
| onClick={handleSaveSites} | ||
| color="primary" | ||
| variant="contained" | ||
| disabled={newSelectedSites.length === 0 || isSaving} | ||
| > | ||
| Save Selected Sites | ||
| {isSaving && <CircularProgress size={24} className={classes.buttonProgress} />} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </Modal> |
There was a problem hiding this comment.
Enhance modal accessibility
The modal component could benefit from improved accessibility features.
Add ARIA attributes and keyboard navigation:
<Modal
className={classes.modal}
open={modalOpen}
onClose={handleCloseModal}
+ aria-labelledby="site-selection-modal-title"
+ aria-describedby="site-selection-modal-description"
>
<div className={classes.modalContent}>
- <Typography variant="h6" gutterBottom>
+ <Typography id="site-selection-modal-title" variant="h6" gutterBottom>
Select sites
</Typography>
+ <Typography id="site-selection-modal-description" variant="body2" gutterBottom>
+ Search and select sites to add to your preferences
+ </Typography>
{/* ... rest of the modal content ... */}
+ <Button onClick={handleCloseModal} color="secondary">
+ Cancel
+ </Button>
</div>
</Modal>📝 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.
| <Modal className={classes.modal} open={modalOpen} onClose={handleCloseModal}> | |
| <div className={classes.modalContent}> | |
| <Typography variant="h6" gutterBottom> | |
| Select sites | |
| </Typography> | |
| <Autocomplete | |
| className={classes.autocomplete} | |
| options={availableSites} | |
| getOptionLabel={(option) => `${option.name} (${option.search_name})`} | |
| value={null} | |
| inputValue={manualSiteInput} | |
| onInputChange={(event, newInputValue) => { | |
| setManualSiteInput(newInputValue); | |
| }} | |
| onChange={handleAddManualSite} | |
| renderInput={(params) => ( | |
| <TextField {...params} label="Enter site name" variant="outlined" /> | |
| )} | |
| /> | |
| <div className={classes.chipContainer}> | |
| {newSelectedSites.map((site) => ( | |
| <Chip | |
| key={site.site_id} | |
| label={`${site.name} (${site.search_name})`} | |
| onDelete={() => handleRemoveNewSite(site)} | |
| className={classes.chip} | |
| /> | |
| ))} | |
| </div> | |
| <div className={classes.wrapper}> | |
| <Button | |
| onClick={handleSaveSites} | |
| color="primary" | |
| variant="contained" | |
| disabled={newSelectedSites.length === 0 || isSaving} | |
| > | |
| Save Selected Sites | |
| {isSaving && <CircularProgress size={24} className={classes.buttonProgress} />} | |
| </Button> | |
| </div> | |
| </div> | |
| </Modal> | |
| <Modal | |
| className={classes.modal} | |
| open={modalOpen} | |
| onClose={handleCloseModal} | |
| aria-labelledby="site-selection-modal-title" | |
| aria-describedby="site-selection-modal-description" | |
| > | |
| <div className={classes.modalContent}> | |
| <Typography id="site-selection-modal-title" variant="h6" gutterBottom> | |
| Select sites | |
| </Typography> | |
| <Typography id="site-selection-modal-description" variant="body2" gutterBottom> | |
| Search and select sites to add to your preferences | |
| </Typography> | |
| <Autocomplete | |
| className={classes.autocomplete} | |
| options={availableSites} | |
| getOptionLabel={(option) => `${option.name} (${option.search_name})`} | |
| value={null} | |
| inputValue={manualSiteInput} | |
| onInputChange={(event, newInputValue) => { | |
| setManualSiteInput(newInputValue); | |
| }} | |
| onChange={handleAddManualSite} | |
| renderInput={(params) => ( | |
| <TextField {...params} label="Enter site name" variant="outlined" /> | |
| )} | |
| /> | |
| <div className={classes.chipContainer}> | |
| {newSelectedSites.map((site) => ( | |
| <Chip | |
| key={site.site_id} | |
| label={`${site.name} (${site.search_name})`} | |
| onDelete={() => handleRemoveNewSite(site)} | |
| className={classes.chip} | |
| /> | |
| ))} | |
| </div> | |
| <div className={classes.wrapper}> | |
| <Button | |
| onClick={handleSaveSites} | |
| color="primary" | |
| variant="contained" | |
| disabled={newSelectedSites.length === 0 || isSaving} | |
| > | |
| Save Selected Sites | |
| {isSaving && <CircularProgress size={24} className={classes.buttonProgress} />} | |
| </Button> | |
| <Button onClick={handleCloseModal} color="secondary"> | |
| Cancel | |
| </Button> | |
| </div> | |
| </div> | |
| </Modal> |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- netmanager/src/views/apis/authService.js (2 hunks)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🔇 Additional comments (4)
netmanager/src/views/apis/authService.js (4)
11-11: LGTM! Clean import of the base URL constant.The import follows the existing patterns and is used consistently throughout the new API functions.
87-91: Previous review comment is still applicable.The earlier suggestions about error handling, constants, and documentation remain valid.
93-99: Previous review comment is still applicable.The earlier suggestions about error handling, input validation, and documentation remain valid.
87-105: Verify the preferences endpoint configuration and usage.Let's ensure the preferences endpoint is properly configured and consistently used across the codebase.
✅ Verification successful
Let me gather more information about the endpoint configuration.
Endpoint configuration and implementation are properly structured
The preferences endpoint implementation is well-organized and follows the established patterns:
- BASE_AUTH_SERVICE_URL_V2 is properly configured in
config/urls/authService.jsusing environment variables- The endpoint follows the same structure as other authenticated user endpoints in the system
- The implementation is the sole consumer of these endpoints, avoiding duplication
- Related user preference hooks and components exist in the codebase, indicating proper integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify preferences endpoint configuration and usage # Test 1: Check if the endpoint is defined in configuration echo "Checking endpoint configuration..." rg -l "users/preferences/selected-sites" "config/" # Test 2: Verify consistent usage of the endpoint echo "Checking endpoint usage..." rg "users/preferences/selected-sites" # Test 3: Look for any potential duplicate implementations echo "Checking for duplicate implementations..." ast-grep --pattern 'createAxiosInstance().get($$$selected-sites$$$)'Length of output: 798
Script:
#!/bin/bash # Check for BASE_AUTH_SERVICE_URL_V2 configuration echo "Checking BASE_AUTH_SERVICE_URL_V2 configuration..." rg "BASE_AUTH_SERVICE_URL_V2" # Check for any related constants or environment variables echo "Checking for related environment configurations..." rg "AUTH_SERVICE.*URL" # Check for any imports of configuration echo "Checking configuration imports..." rg "import.*config" # Look for any preference-related components or hooks echo "Checking for preference-related usage..." rg "userPreference" -t jsLength of output: 14983
| export const deleteUserPreferenceSiteApi = async (siteId) => { | ||
| return await createAxiosInstance() | ||
| .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the delete API function with proper error handling and documentation.
The implementation could benefit from similar improvements as suggested for the other preference API functions.
Consider applying this enhancement:
+const PREFERENCES_ENDPOINT = '/users/preferences/selected-sites';
+/**
+ * Deletes a specific site from user preferences
+ * @param {string} siteId - ID of the site to remove
+ * @returns {Promise<{message: string}>} Success response
+ * @throws {Error} When siteId is invalid
+ * @throws {AxiosError} When the request fails
+ */
export const deleteUserPreferenceSiteApi = async (siteId) => {
+ if (!siteId) {
+ throw new Error('siteId is required');
+ }
+
+ try {
return await createAxiosInstance()
- .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`)
+ .delete(`${BASE_AUTH_SERVICE_URL_V2}${PREFERENCES_ENDPOINT}/${siteId}`)
.then((response) => response.data);
+ } catch (error) {
+ console.error('Failed to delete user preference site:', error);
+ throw error;
+ }
};📝 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.
| export const deleteUserPreferenceSiteApi = async (siteId) => { | |
| return await createAxiosInstance() | |
| .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`) | |
| .then((response) => response.data); | |
| }; | |
| const PREFERENCES_ENDPOINT = '/users/preferences/selected-sites'; | |
| /** | |
| * Deletes a specific site from user preferences | |
| * @param {string} siteId - ID of the site to remove | |
| * @returns {Promise<{message: string}>} Success response | |
| * @throws {Error} When siteId is invalid | |
| * @throws {AxiosError} When the request fails | |
| */ | |
| export const deleteUserPreferenceSiteApi = async (siteId) => { | |
| if (!siteId) { | |
| throw new Error('siteId is required'); | |
| } | |
| try { | |
| return await createAxiosInstance() | |
| .delete(`${BASE_AUTH_SERVICE_URL_V2}${PREFERENCES_ENDPOINT}/${siteId}`) | |
| .then((response) => response.data); | |
| } catch (error) { | |
| console.error('Failed to delete user preference site:', error); | |
| throw error; | |
| } | |
| }; |
| {isLoading ? ( | ||
| <div className={classes.loaderContainer}> | ||
| <CircularProgress /> | ||
| </div> | ||
| ) : selectedSites.length === 0 ? ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading state UX with skeleton
Instead of showing just a CircularProgress, consider using Material-UI's Skeleton component to provide a better loading experience that maintains the layout structure.
+import Skeleton from '@material-ui/lab/Skeleton';
{isLoading ? (
- <div className={classes.loaderContainer}>
- <CircularProgress />
- </div>
+ <Grid container spacing={2}>
+ {[1, 2, 3].map((key) => (
+ <Grid item xs={12} sm={6} md={4} key={key}>
+ <Skeleton variant="rect" height={140} />
+ </Grid>
+ ))}
+ </Grid>
)Committable suggestion was skipped due to low confidence.
| const handleSaveSites = async () => { | ||
| setIsSaving(true); | ||
| setError(null); | ||
| try { | ||
| const updatedSites = [...selectedSites, ...newSelectedSites]; | ||
| await setUserPreferencesApi(updatedSites); | ||
| setSelectedSites(updatedSites); | ||
| setModalOpen(false); | ||
| showSnackbar('Sites updated successfully'); | ||
| } catch (error) { | ||
| console.error('Error updating sites:', error); | ||
| const errorMessage = | ||
| error.response?.data?.message || error.message || 'An unknown error occurred'; | ||
| setError(`Failed to update sites: ${errorMessage}`); | ||
| showSnackbar(`Error updating sites: ${errorMessage}`, 'error'); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add timeout handling for site updates
The save operation could potentially hang indefinitely if the network request doesn't complete. Consider adding a timeout to prevent the UI from being stuck in a loading state.
const handleSaveSites = async () => {
setIsSaving(true);
setError(null);
try {
+ const timeoutPromise = new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('Request timed out')), 30000)
+ );
+ const updatePromise = setUserPreferencesApi(updatedSites);
+ await Promise.race([updatePromise, timeoutPromise]);
- await setUserPreferencesApi(updatedSites);
setSelectedSites(updatedSites);
setModalOpen(false);
showSnackbar('Sites updated successfully');
} catch (error) {
console.error('Error updating sites:', error);
const errorMessage =
- error.response?.data?.message || error.message || 'An unknown error occurred';
+ error.message === 'Request timed out'
+ ? 'Operation timed out. Please try again.'
+ : error.response?.data?.message || error.message || 'An unknown error occurred';
setError(`Failed to update sites: ${errorMessage}`);
showSnackbar(`Error updating sites: ${errorMessage}`, 'error');
} finally {
setIsSaving(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.
| const handleSaveSites = async () => { | |
| setIsSaving(true); | |
| setError(null); | |
| try { | |
| const updatedSites = [...selectedSites, ...newSelectedSites]; | |
| await setUserPreferencesApi(updatedSites); | |
| setSelectedSites(updatedSites); | |
| setModalOpen(false); | |
| showSnackbar('Sites updated successfully'); | |
| } catch (error) { | |
| console.error('Error updating sites:', error); | |
| const errorMessage = | |
| error.response?.data?.message || error.message || 'An unknown error occurred'; | |
| setError(`Failed to update sites: ${errorMessage}`); | |
| showSnackbar(`Error updating sites: ${errorMessage}`, 'error'); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; | |
| const handleSaveSites = async () => { | |
| setIsSaving(true); | |
| setError(null); | |
| try { | |
| const timeoutPromise = new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('Request timed out')), 30000) | |
| ); | |
| const updatePromise = setUserPreferencesApi(updatedSites); | |
| await Promise.race([updatePromise, timeoutPromise]); | |
| setSelectedSites(updatedSites); | |
| setModalOpen(false); | |
| showSnackbar('Sites updated successfully'); | |
| } catch (error) { | |
| console.error('Error updating sites:', error); | |
| const errorMessage = | |
| error.message === 'Request timed out' | |
| ? 'Operation timed out. Please try again.' | |
| : error.response?.data?.message || error.message || 'An unknown error occurred'; | |
| setError(`Failed to update sites: ${errorMessage}`); | |
| showSnackbar(`Error updating sites: ${errorMessage}`, 'error'); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🔇 Additional comments (3)
netmanager/src/views/pages/Preferences/Preferences.js (3)
178-183:⚠️ Potential issueAdd missing dependency to useEffect
The effect depends on
allSites.lengthbut it's not included in the dependency array. This could lead to missed updates.useEffect(() => { fetchSelectedSites(); if (allSites.length === 0 && activeNetwork) { dispatch(loadSitesSummary(activeNetwork.net_name)); } - }, [dispatch, activeNetwork]); + }, [dispatch, activeNetwork, allSites.length]);Likely invalid or redundant comment.
32-154: 🛠️ Refactor suggestionOptimize style performance with useMemo
Consider memoizing the styles object to prevent unnecessary recalculations.
-const useStyles = makeStyles((theme) => ({ +const useStyles = makeStyles((theme) => useMemo(() => ({ // ... style definitions -})); +}), []));Likely invalid or redundant comment.
227-238:⚠️ Potential issuePrevent duplicate site selection
The
handleAddManualSitefunction should check for duplicates innewSelectedSitesbefore adding a site.const handleAddManualSite = (event, newValue) => { if (newValue) { + const isDuplicate = newSelectedSites.some(site => site.site_id === newValue._id); + if (isDuplicate) { + showSnackbar('Site already selected', 'warning'); + return; + } const { _id, name, search_name } = newValue; const newSite = { site_id: _id, name, search_name }; setNewSelectedSites([...newSelectedSites, newSite]); setManualSiteInput(''); } };Likely invalid or redundant comment.
| {selectedSites.map((site) => ( | ||
| <Grid item xs={12} sm={6} md={4} key={site.site_id}> | ||
| <Card className={classes.card}> | ||
| <CardContent className={classes.cardContent}> | ||
| <Typography variant="subtitle1" className={classes.siteName}> | ||
| {site.name} | ||
| </Typography> | ||
| <Typography variant="body2" className={classes.siteSearchName}> | ||
| {site.search_name} | ||
| </Typography> | ||
| </CardContent> | ||
| <CardActions className={classes.cardActions}> | ||
| <Button | ||
| startIcon={<DeleteIcon />} | ||
| onClick={() => handleRemoveSite(site)} | ||
| color="secondary" | ||
| size="small" | ||
| disabled={removingSites.has(site.site_id)} | ||
| > | ||
| {removingSites.has(site.site_id) ? 'Removing...' : 'Remove'} | ||
| </Button> | ||
| </CardActions> | ||
| </Card> | ||
| </Grid> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize list rendering with virtualization
For better performance with large lists, consider implementing virtualization for the site grid.
Consider using react-window or react-virtualized to handle large lists of sites efficiently. This will improve performance by only rendering the visible items.
Example implementation with react-window:
import { FixedSizeGrid as Grid } from 'react-window';
const SiteGrid = ({ sites, columnCount }) => (
<Grid
columnCount={columnCount}
columnWidth={300}
height={600}
rowCount={Math.ceil(sites.length / columnCount)}
rowHeight={200}
width={columnCount * 300}
>
{({ columnIndex, rowIndex, style }) => {
const site = sites[rowIndex * columnCount + columnIndex];
return site ? (
<div style={style}>
<SiteCard site={site} />
</div>
) : null;
}}
</Grid>
);| const handleTabKey = (e) => { | ||
| const focusableModalElements = modalRef.current.querySelectorAll( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); | ||
| const firstElement = focusableModalElements[0]; | ||
| const lastElement = focusableModalElements[focusableModalElements.length - 1]; | ||
|
|
||
| if (!e.shiftKey && document.activeElement === lastElement) { | ||
| firstElement.focus(); | ||
| e.preventDefault(); | ||
| } | ||
|
|
||
| if (e.shiftKey && document.activeElement === firstElement) { | ||
| lastElement.focus(); | ||
| e.preventDefault(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keyboard navigation with focus trap
Consider using a focus trap library like focus-trap-react for better modal keyboard navigation.
+import FocusTrap from 'focus-trap-react';
-<div
+<FocusTrap
className={classes.modalContent}
ref={modalRef}
tabIndex={-1}
onKeyDown={handleKeyDown}
role="dialog"
aria-modal="true"
->
+>
{/* modal content */}
-</div>
+</FocusTrap>Committable suggestion was skipped due to low confidence.
| import React, { useState, useEffect, useMemo } from 'react'; | ||
| import { makeStyles } from '@material-ui/core/styles'; | ||
| import { | ||
| Container, | ||
| Typography, | ||
| Paper, | ||
| Grid, | ||
| Card, | ||
| CardContent, | ||
| CardActions, | ||
| Button, | ||
| Snackbar, | ||
| Modal, | ||
| Chip, | ||
| CircularProgress, | ||
| Box | ||
| } from '@material-ui/core'; | ||
| import { Skeleton } from '@material-ui/lab'; | ||
| import TextField from '@material-ui/core/TextField'; | ||
| import AddIcon from '@material-ui/icons/Add'; | ||
| import DeleteIcon from '@material-ui/icons/Delete'; | ||
| import { | ||
| getUserPreferencesApi, | ||
| setUserPreferencesApi, | ||
| deleteUserPreferenceSiteApi | ||
| } from 'views/apis/authService'; | ||
| import { useSitesSummaryData } from 'redux/SiteRegistry/selectors'; | ||
| import { Alert, Autocomplete } from '@material-ui/lab'; | ||
| import { useDispatch, useSelector } from 'react-redux'; | ||
| import { loadSitesSummary } from 'redux/SiteRegistry/operations'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing bundle size with selective imports
Instead of importing the entire @material-ui/core package, consider importing components selectively to reduce bundle size.
-import {
- Container,
- Typography,
- Paper,
- Grid,
- Card,
- CardContent,
- CardActions,
- Button,
- Snackbar,
- Modal,
- Chip,
- CircularProgress,
- Box
-} from '@material-ui/core';
+import Container from '@material-ui/core/Container';
+import Typography from '@material-ui/core/Typography';
+import Paper from '@material-ui/core/Paper';
+import Grid from '@material-ui/core/Grid';
+import Card from '@material-ui/core/Card';
+import CardContent from '@material-ui/core/CardContent';
+import CardActions from '@material-ui/core/CardActions';
+import Button from '@material-ui/core/Button';
+import Snackbar from '@material-ui/core/Snackbar';
+import Modal from '@material-ui/core/Modal';
+import Chip from '@material-ui/core/Chip';
+import CircularProgress from '@material-ui/core/CircularProgress';
+import Box from '@material-ui/core/Box';📝 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.
| import React, { useState, useEffect, useMemo } from 'react'; | |
| import { makeStyles } from '@material-ui/core/styles'; | |
| import { | |
| Container, | |
| Typography, | |
| Paper, | |
| Grid, | |
| Card, | |
| CardContent, | |
| CardActions, | |
| Button, | |
| Snackbar, | |
| Modal, | |
| Chip, | |
| CircularProgress, | |
| Box | |
| } from '@material-ui/core'; | |
| import { Skeleton } from '@material-ui/lab'; | |
| import TextField from '@material-ui/core/TextField'; | |
| import AddIcon from '@material-ui/icons/Add'; | |
| import DeleteIcon from '@material-ui/icons/Delete'; | |
| import { | |
| getUserPreferencesApi, | |
| setUserPreferencesApi, | |
| deleteUserPreferenceSiteApi | |
| } from 'views/apis/authService'; | |
| import { useSitesSummaryData } from 'redux/SiteRegistry/selectors'; | |
| import { Alert, Autocomplete } from '@material-ui/lab'; | |
| import { useDispatch, useSelector } from 'react-redux'; | |
| import { loadSitesSummary } from 'redux/SiteRegistry/operations'; | |
| import React, { useState, useEffect, useMemo } from 'react'; | |
| import { makeStyles } from '@material-ui/core/styles'; | |
| import Container from '@material-ui/core/Container'; | |
| import Typography from '@material-ui/core/Typography'; | |
| import Paper from '@material-ui/core/Paper'; | |
| import Grid from '@material-ui/core/Grid'; | |
| import Card from '@material-ui/core/Card'; | |
| import CardContent from '@material-ui/core/CardContent'; | |
| import CardActions from '@material-ui/core/CardActions'; | |
| import Button from '@material-ui/core/Button'; | |
| import Snackbar from '@material-ui/core/Snackbar'; | |
| import Modal from '@material-ui/core/Modal'; | |
| import Chip from '@material-ui/core/Chip'; | |
| import CircularProgress from '@material-ui/core/CircularProgress'; | |
| import Box from '@material-ui/core/Box'; | |
| import { Skeleton } from '@material-ui/lab'; | |
| import TextField from '@material-ui/core/TextField'; | |
| import AddIcon from '@material-ui/icons/Add'; | |
| import DeleteIcon from '@material-ui/icons/Delete'; | |
| import { | |
| getUserPreferencesApi, | |
| setUserPreferencesApi, | |
| deleteUserPreferenceSiteApi | |
| } from 'views/apis/authService'; | |
| import { useSitesSummaryData } from 'redux/SiteRegistry/selectors'; | |
| import { Alert, Autocomplete } from '@material-ui/lab'; | |
| import { useDispatch, useSelector } from 'react-redux'; | |
| import { loadSitesSummary } from 'redux/SiteRegistry/operations'; |
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
1 similar comment
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
netmanager/src/views/pages/Preferences/Preferences.js (1)
171-171: Consider adding error boundary and fallback UIThe component handles individual operation errors but lacks global error handling. Consider wrapping it in an error boundary to gracefully handle rendering failures.
I can help implement an error boundary component if you'd like. Would you like me to create a new issue for this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- netmanager/src/AppRoutes.js (3 hunks)
- netmanager/src/views/apis/authService.js (2 hunks)
- netmanager/src/views/layouts/common/Sidebar/Sidebar.js (1 hunks)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- netmanager/src/AppRoutes.js
- netmanager/src/views/layouts/common/Sidebar/Sidebar.js
🧰 Additional context used
🪛 Biome
netmanager/src/views/pages/Preferences/Preferences.js
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
netmanager/src/views/apis/authService.js (3)
11-11: LGTM! Clean import addition.The import of BASE_AUTH_SERVICE_URL_V2 is properly placed and consistently used throughout the new API functions.
87-91: Consider implementing caching for preferences.The function implementation could benefit from caching to improve performance and reduce API calls, especially since preferences don't change frequently.
Consider implementing a simple cache:
let preferencesCache = null; let lastFetchTime = null; const CACHE_DURATION = 5 * 60 * 1000; // 5 minutes export const getDefaultSelectedSitesApi = async () => { if (preferencesCache && lastFetchTime && Date.now() - lastFetchTime < CACHE_DURATION) { return preferencesCache; } const response = await createAxiosInstance() .get(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`) .then(response => response.data); preferencesCache = response; lastFetchTime = Date.now(); return response; };
93-99:⚠️ Potential issueMaintain consistent naming conventions.
The payload uses snake_case (
selected_sites) while the parameter uses camelCase (selectedSites). Consider maintaining consistency with the frontend's camelCase convention and let the API handle the conversion if needed.export const setDefaultSelectedSitesApi = async (selectedSites) => { return await createAxiosInstance() .post(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites`, { - selected_sites: selectedSites + selectedSites }) .then((response) => response.data); };
| export const deleteDefaultSelectedSiteApi = async (siteId) => { | ||
| return await createAxiosInstance() | ||
| .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
Protect against XSS in URL parameters.
While using template literals for URLs, ensure the siteId is properly encoded to prevent XSS attacks.
export const deleteDefaultSelectedSiteApi = async (siteId) => {
return await createAxiosInstance()
.delete(
- `${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`
+ `${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${encodeURIComponent(siteId)}`
)
.then((response) => response.data);
};📝 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.
| export const deleteDefaultSelectedSiteApi = async (siteId) => { | |
| return await createAxiosInstance() | |
| .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`) | |
| .then((response) => response.data); | |
| }; | |
| export const deleteDefaultSelectedSiteApi = async (siteId) => { | |
| return await createAxiosInstance() | |
| .delete(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${encodeURIComponent(siteId)}`) | |
| .then((response) => response.data); | |
| }; |
| export const updateDefaultSelectedSiteApi = async (siteId, siteData) => { | ||
| return await createAxiosInstance() | ||
| .put(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`, siteData) | ||
| .then((response) => response.data); | ||
| }; |
There was a problem hiding this comment.
Add input validation and URL encoding.
The function should validate siteData and encode URL parameters.
export const updateDefaultSelectedSiteApi = async (siteId, siteData) => {
+ if (!siteData || typeof siteData !== 'object') {
+ throw new Error('Invalid site data provided');
+ }
return await createAxiosInstance()
.put(
- `${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`,
+ `${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${encodeURIComponent(siteId)}`,
siteData
)
.then((response) => response.data);
};📝 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.
| export const updateDefaultSelectedSiteApi = async (siteId, siteData) => { | |
| return await createAxiosInstance() | |
| .put(`${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${siteId}`, siteData) | |
| .then((response) => response.data); | |
| }; | |
| export const updateDefaultSelectedSiteApi = async (siteId, siteData) => { | |
| if (!siteData || typeof siteData !== 'object') { | |
| throw new Error('Invalid site data provided'); | |
| } | |
| return await createAxiosInstance() | |
| .put( | |
| `${BASE_AUTH_SERVICE_URL_V2}/users/preferences/selected-sites/${encodeURIComponent(siteId)}`, | |
| siteData | |
| ) | |
| .then((response) => response.data); | |
| }; |
| if (response && response.selected_sites) { | ||
| setSelectedSites(response.selected_sites); | ||
| } else { | ||
| setSelectedSites([]); | ||
| console.warn('No selected sites data in the response'); | ||
| } |
There was a problem hiding this comment.
Enhance null check safety with optional chaining
The response object access could be safer using optional chaining.
- if (response && response.selected_sites) {
+ if (response?.selected_sites) {
setSelectedSites(response.selected_sites);
} else {
setSelectedSites([]);
console.warn('No selected sites data in the response');
}📝 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.
| if (response && response.selected_sites) { | |
| setSelectedSites(response.selected_sites); | |
| } else { | |
| setSelectedSites([]); | |
| console.warn('No selected sites data in the response'); | |
| } | |
| if (response?.selected_sites) { | |
| setSelectedSites(response.selected_sites); | |
| } else { | |
| setSelectedSites([]); | |
| console.warn('No selected sites data in the response'); | |
| } |
🧰 Tools
🪛 Biome
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| onClick={() => handleRemoveSite(site)} | ||
| color="secondary" | ||
| size="small" | ||
| disabled={true} // Disabled for now |
There was a problem hiding this comment.
Remove button is permanently disabled
The remove functionality is implemented but the button is always disabled, making it impossible for users to remove sites.
- disabled={true} // Disabled for now
+ disabled={removingSites.has(site.site_id)}Committable suggestion was skipped due to low confidence.
| const handleToggleFeatured = async (site) => { | ||
| setUpdatingSiteId(site.site_id); | ||
| setError(null); | ||
| try { | ||
| const updatedSites = selectedSites.map((s) => ({ | ||
| site_id: s.site_id, | ||
| isFeatured: s.site_id === site.site_id ? !s.isFeatured : false | ||
| })); | ||
|
|
||
| // Call API to update all sites | ||
| await Promise.all(updatedSites.map((s) => updateDefaultSelectedSiteApi(s.site_id, s))); | ||
|
|
||
| setSelectedSites(updatedSites); | ||
| showSnackbar('Site updated successfully'); | ||
|
|
||
| // Refresh the default sites cards | ||
| await fetchSelectedSites(); | ||
| } catch (error) { | ||
| console.error('Error updating site:', error); | ||
| const errorMessage = | ||
| error.response?.data?.message || error.message || 'An unknown error occurred'; | ||
| setError(`Failed to update site: ${errorMessage}`); | ||
| showSnackbar(`Error updating site: ${errorMessage}`, 'error'); | ||
| } finally { | ||
| setUpdatingSiteId(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize featured site toggle implementation
The current implementation has several issues:
- Unnecessary API calls for all sites when toggling one site
- Race condition potential with multiple rapid toggles
- Redundant fetchSelectedSites call after state update
Consider this optimized implementation:
const handleToggleFeatured = async (site) => {
setUpdatingSiteId(site.site_id);
setError(null);
try {
- const updatedSites = selectedSites.map((s) => ({
- site_id: s.site_id,
- isFeatured: s.site_id === site.site_id ? !s.isFeatured : false
- }));
-
- // Call API to update all sites
- await Promise.all(updatedSites.map((s) => updateDefaultSelectedSiteApi(s.site_id, s)));
-
+ const updatedSite = {
+ site_id: site.site_id,
+ isFeatured: !site.isFeatured
+ };
+ await updateDefaultSelectedSiteApi(site.site_id, updatedSite);
+
+ setSelectedSites(prev => prev.map(s =>
+ s.site_id === site.site_id
+ ? { ...s, isFeatured: !s.isFeatured }
+ : { ...s, isFeatured: false }
+ ));
setSelectedSites(updatedSites);
showSnackbar('Site updated successfully');
-
- // Refresh the default sites cards
- await fetchSelectedSites();
} catch (error) {
console.error('Error updating site:', error);
const errorMessage =
error.response?.data?.message || error.message || 'An unknown error occurred';
setError(`Failed to update site: ${errorMessage}`);
showSnackbar(`Error updating site: ${errorMessage}`, 'error');
} finally {
setUpdatingSiteId(null);
}
};Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🧰 Additional context used
🪛 Biome
netmanager/src/views/pages/Preferences/Preferences.js
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
netmanager/src/views/pages/Preferences/Preferences.js (1)
204-209:⚠️ Potential issueUse optional chaining for safer response handling
-if (response && response.selected_sites) { +if (response?.selected_sites) { setSelectedSites(response.selected_sites); } else { setSelectedSites([]); console.warn('No selected sites data in the response'); }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| <Modal | ||
| className={classes.modal} | ||
| open={modalOpen} | ||
| onClose={handleCloseModal} | ||
| aria-labelledby="modal-title" | ||
| aria-describedby="modal-description" | ||
| > | ||
| <div | ||
| className={classes.modalContent} | ||
| ref={modalRef} | ||
| tabIndex={-1} | ||
| onKeyDown={handleKeyDown} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance modal accessibility with focus management
While the modal has basic accessibility attributes, it could be improved by:
- Managing focus when the modal opens
- Trapping focus within the modal
- Restoring focus when the modal closes
Consider using a focus management library like focus-trap-react:
+import FocusTrap from 'focus-trap-react';
-<div
+<FocusTrap
+ focusTrapOptions={{
+ initialFocus: '#modal-title',
+ returnFocusOnDeactivate: true
+ }}
className={classes.modalContent}
ref={modalRef}
tabIndex={-1}
onKeyDown={handleKeyDown}
role="dialog"
aria-modal="true"
->Committable suggestion was skipped due to low confidence.
| const [isLoading, setIsLoading] = useState(true); | ||
| const [isSaving, setIsSaving] = useState(false); | ||
| const [error, setError] = useState(null); | ||
| const [removingSites, setRemovingSites] = useState(new Set()); | ||
| const [updatingSiteId, setUpdatingSiteId] = useState(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating loading states
The component maintains separate loading states (isLoading, isSaving) and error states. Consider combining them into a single state object for better maintainability:
-const [isLoading, setIsLoading] = useState(true);
-const [isSaving, setIsSaving] = useState(false);
-const [error, setError] = useState(null);
+const [status, setStatus] = useState({
+ isLoading: true,
+ isSaving: false,
+ error: null
+});📝 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 [isLoading, setIsLoading] = useState(true); | |
| const [isSaving, setIsSaving] = useState(false); | |
| const [error, setError] = useState(null); | |
| const [removingSites, setRemovingSites] = useState(new Set()); | |
| const [updatingSiteId, setUpdatingSiteId] = useState(null); | |
| const [status, setStatus] = useState({ | |
| isLoading: true, | |
| isSaving: false, | |
| error: null | |
| }); | |
| const [removingSites, setRemovingSites] = useState(new Set()); | |
| const [updatingSiteId, setUpdatingSiteId] = useState(null); |
| const availableSites = useMemo(() => { | ||
| const selectedSiteIds = new Set(selectedSites.map((site) => site.site_id)); | ||
| const uniqueSearchNames = new Set(); | ||
| return allSites.filter((site) => { | ||
| if (!site.search_name || selectedSiteIds.has(site._id) || !site.isOnline || site.isFeatured) { | ||
| return false; | ||
| } | ||
| if (uniqueSearchNames.has(site.search_name)) { | ||
| return false; | ||
| } | ||
| uniqueSearchNames.add(site.search_name); | ||
| return true; | ||
| }); | ||
| }, [allSites, selectedSites]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize site filtering logic
The current filtering logic could be more efficient by:
- Using a single pass through the data
- Avoiding multiple Set operations
const availableSites = useMemo(() => {
const selectedSiteIds = new Set(selectedSites.map((site) => site.site_id));
const uniqueSearchNames = new Set();
- return allSites.filter((site) => {
- if (!site.search_name || selectedSiteIds.has(site._id) || !site.isOnline || site.isFeatured) {
- return false;
- }
- if (uniqueSearchNames.has(site.search_name)) {
- return false;
- }
- uniqueSearchNames.add(site.search_name);
- return true;
- });
+ return allSites.reduce((acc, site) => {
+ if (
+ site.search_name &&
+ !selectedSiteIds.has(site._id) &&
+ site.isOnline &&
+ !site.isFeatured &&
+ !uniqueSearchNames.has(site.search_name)
+ ) {
+ uniqueSearchNames.add(site.search_name);
+ acc.push(site);
+ }
+ return acc;
+ }, []);
}, [allSites, selectedSites]);📝 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 availableSites = useMemo(() => { | |
| const selectedSiteIds = new Set(selectedSites.map((site) => site.site_id)); | |
| const uniqueSearchNames = new Set(); | |
| return allSites.filter((site) => { | |
| if (!site.search_name || selectedSiteIds.has(site._id) || !site.isOnline || site.isFeatured) { | |
| return false; | |
| } | |
| if (uniqueSearchNames.has(site.search_name)) { | |
| return false; | |
| } | |
| uniqueSearchNames.add(site.search_name); | |
| return true; | |
| }); | |
| }, [allSites, selectedSites]); | |
| const availableSites = useMemo(() => { | |
| const selectedSiteIds = new Set(selectedSites.map((site) => site.site_id)); | |
| const uniqueSearchNames = new Set(); | |
| return allSites.reduce((acc, site) => { | |
| if ( | |
| site.search_name && | |
| !selectedSiteIds.has(site._id) && | |
| site.isOnline && | |
| !site.isFeatured && | |
| !uniqueSearchNames.has(site.search_name) | |
| ) { | |
| uniqueSearchNames.add(site.search_name); | |
| acc.push(site); | |
| } | |
| return acc; | |
| }, []); | |
| }, [allSites, selectedSites]); |
|
New netmanager changes available for preview here |
2 similar comments
|
New netmanager changes available for preview here |
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Thanks @Codebmk , This is great progress. The layout looks amazing!
We might need to review the functionality of the entire page. It seems as though things keep reloading with no end and when they do end loading, they bring up a network error. And then after the the list of the default preferences disappears completely!
|
New netmanager changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
netmanager/src/redux/DefaultSitePreferences/operations.js (1)
56-65: Add input validation and consider type documentation.
The implementation is solid, but would benefit from parameter validation and TypeScript-style documentation for better maintainability.
+/**
+ * Updates a default site
+ * @param {string} siteId - The ID of the site to update
+ * @param {Object} siteData - The new site data
+ * @returns {Promise<Object>} The updated site object
+ */
export const updateDefaultSite = (siteId, siteData) => async (dispatch) => {
try {
+ if (!siteId) {
+ throw new Error('Site ID is required');
+ }
+ if (!siteData || typeof siteData !== 'object') {
+ throw new Error('Site data must be an object');
+ }
await updateDefaultSelectedSiteApi(siteId, siteData);
dispatch(updateDefaultSiteSuccess({ site_id: siteId, ...siteData }));
return { site_id: siteId, ...siteData };
} catch (error) {📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- netmanager/src/redux/DefaultSitePreferences/actions.js (1 hunks)
- netmanager/src/redux/DefaultSitePreferences/operations.js (1 hunks)
- netmanager/src/redux/DefaultSitePreferences/reducer.js (1 hunks)
- netmanager/src/redux/index.js (2 hunks)
- netmanager/src/views/pages/Preferences/Preferences.js (1 hunks)
🧰 Additional context used
🪛 Biome
netmanager/src/redux/DefaultSitePreferences/operations.js
[error] 21-21: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
netmanager/src/redux/DefaultSitePreferences/reducer.js (1)
20-48: 🛠️ Refactor suggestion
Add payload validation for success cases.
The reducer assumes action.payload will always be in the correct format. For robustness, consider adding validation to prevent runtime errors.
Let's verify the payload structure in the actions file:
Add validation before state updates:
case LOAD_DEFAULT_SITES_SUCCESS:
+ if (!Array.isArray(action.payload)) {
+ return {
+ ...state,
+ error: 'Invalid payload format for sites'
+ };
+ }
return {
...state,
sites: action.payload,
error: null
};netmanager/src/redux/DefaultSitePreferences/actions.js (2)
1-11: Well-structured action type constants!
The action types follow Redux naming conventions and are logically grouped by operation type, making the code easy to understand and maintain.
1-51: Verify integration with reducer and operations.
Let's ensure these actions are properly integrated with the rest of the Redux flow.
✅ Verification successful
Redux integration looks complete and well-structured
The verification shows proper integration:
- All action types have corresponding reducer cases in
reducer.js - All success/failure action creators are imported and dispatched in
operations.js - The action flow follows Redux best practices with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for corresponding reducer cases
echo "Checking reducer implementation..."
rg -A 2 "case ['\"]?(LOAD_DEFAULT_SITES|SET_DEFAULT_SITES|DELETE_DEFAULT_SITE|UPDATE_DEFAULT_SITE)" netmanager/src/redux/DefaultSitePreferences/
# Check for action usage in operations
echo "Checking operations usage..."
rg "(loadDefaultSites|setDefaultSites|deleteDefaultSite|updateDefaultSite)(Success|Failure)" netmanager/src/redux/DefaultSitePreferences/operations.js
Length of output: 2592
netmanager/src/redux/DefaultSitePreferences/operations.js (2)
1-16: LGTM! Clean and well-organized imports.
The imports are logically grouped between external API functions and local action creators.
1-65: Verify error handling in the UI components.
The operations consistently throw errors after dispatching failure actions. Ensure that components using these operations properly catch and handle these errors to provide appropriate user feedback.
✅ Verification successful
Let me check the error handling implementation in the components.
Error handling is properly implemented in the UI components ✅
The Preferences component demonstrates robust error handling for all operations:
- Uses try-catch blocks consistently
- Extracts detailed error messages from API responses
- Shows user-friendly error notifications via snackbar
- Manages loading states appropriately
- Includes cleanup with finally blocks where needed
The implementation aligns perfectly with the error throwing behavior in the operations file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using these operations to verify error handling
rg -l "loadDefaultSites|setDefaultSites|deleteDefaultSite|updateDefaultSite" --type js
Length of output: 255
Script:
#!/bin/bash
# Check error handling in Preferences.js
rg "loadDefaultSites|setDefaultSites|deleteDefaultSite|updateDefaultSite" -A 5 -B 5 netmanager/src/views/pages/Preferences/Preferences.js
Length of output: 2036
🧰 Tools
🪛 Biome
[error] 21-21: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
netmanager/src/redux/index.js (2)
23-23: Clean import statement, well placed!
The import follows the established pattern and is appropriately placed with other reducer imports.
46-47: Reducer integration looks good, let's verify the implementation.
The new reducer is properly integrated into the root reducer with consistent naming. The comma addition maintains clean syntax.
Let's verify the reducer implementation and its associated files:
✅ Verification successful
Reducer implementation verified and looks solid ✅
The defaultSitePreferences reducer is well-implemented with:
- Clean initial state structure
- Proper handling of success/failure cases for all operations
- Immutable state updates following Redux best practices
- Consistent error handling across all failure cases
The reducer is also properly connected in both the root reducer and the Preferences component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete implementation of the default site preferences feature
# Check if the reducer file exists and examine its structure
echo "Checking reducer implementation..."
fd "reducer.js" --type f --full-path "DefaultSitePreferences"
# Look for corresponding action creators and types
echo "Checking for action creators..."
fd "actions.js" --type f --full-path "DefaultSitePreferences"
# Verify the operations/thunks implementation
echo "Checking for operations/thunks..."
fd "operations.js" --type f --full-path "DefaultSitePreferences"
# Look for any associated selectors
echo "Checking for selectors..."
fd "selectors.js" --type f --full-path "DefaultSitePreferences"
# Examine the initial state and action handlers
echo "Examining reducer implementation..."
rg -A 10 "initialState|case" "DefaultSitePreferences/reducer.js"
Length of output: 1194
Script:
#!/bin/bash
# Let's try a different approach to locate the reducer implementation
# First, let's find all reducer files in the codebase
echo "Looking for reducer files..."
fd "reducer.js" --type f
# Then search for defaultSitePreferences related code
echo "\nSearching for defaultSitePreferences implementation..."
rg -l "defaultSitePreferences" --type js
# Look for the actual reducer implementation
echo "\nSearching for reducer implementation..."
rg "defaultSitePreferences.*reducer" --type js -A 5
Length of output: 1514
Script:
#!/bin/bash
# Let's examine the DefaultSitePreferences reducer implementation
# Check the reducer content
echo "Examining DefaultSitePreferences reducer..."
cat netmanager/src/redux/DefaultSitePreferences/reducer.js
# Look for related actions and types
echo "\nLooking for related actions..."
fd actions.js --parent-directory netmanager/src/redux/DefaultSitePreferences
# Check for usage in Preferences component
echo "\nChecking usage in Preferences component..."
rg -A 5 "defaultSitePreferences" netmanager/src/views/pages/Preferences/Preferences.js
Length of output: 2433
netmanager/src/views/pages/Preferences/Preferences.js (4)
3-17: Consider optimizing bundle size with selective imports
A past review comment about optimizing bundle size through selective imports is still applicable here.
198-203: Include allSites.length in useEffect dependencies
A past review comment about missing allSites.length in the dependency array is still applicable here.
421-429: Remove button is permanently disabled
A past review comment about the permanently disabled remove button is still applicable here.
457-471: Enhance modal accessibility with focus management
A past review comment about improving modal accessibility with focus management is still applicable here.
| case LOAD_DEFAULT_SITES_FAILURE: | ||
| case SET_DEFAULT_SITES_FAILURE: | ||
| case DELETE_DEFAULT_SITE_FAILURE: | ||
| case UPDATE_DEFAULT_SITE_FAILURE: | ||
| return { | ||
| ...state, | ||
| error: action.payload | ||
| }; | ||
|
|
||
| default: | ||
| return state; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling specificity.
Currently, all failure cases are handled identically. Consider adding specific error handling for different types of failures to improve debugging and user feedback.
Here's a more specific approach:
- case LOAD_DEFAULT_SITES_FAILURE:
- case SET_DEFAULT_SITES_FAILURE:
- case DELETE_DEFAULT_SITE_FAILURE:
- case UPDATE_DEFAULT_SITE_FAILURE:
- return {
- ...state,
- error: action.payload
- };
+ case LOAD_DEFAULT_SITES_FAILURE:
+ return {
+ ...state,
+ error: {
+ type: 'LOAD_ERROR',
+ message: action.payload,
+ timestamp: new Date().toISOString()
+ }
+ };
+ // Add similar specific cases for other failuresCommittable suggestion was skipped due to low confidence.
| const initialState = { | ||
| sites: [], | ||
| loading: false, | ||
| error: null | ||
| }; |
There was a problem hiding this comment.
Utilize the loading flag in reducer cases.
The initial state includes a loading flag, but it's not being updated in any reducer cases. This flag should be toggled during request/success/failure operations.
Here's how to utilize the loading flag:
case LOAD_DEFAULT_SITES_SUCCESS:
return {
...state,
sites: action.payload,
+ loading: false,
error: null
};
+case LOAD_DEFAULT_SITES_REQUEST:
+ return {
+ ...state,
+ loading: true,
+ error: null
+ };
case LOAD_DEFAULT_SITES_FAILURE:
return {
...state,
+ loading: false,
error: action.payload
};Committable suggestion was skipped due to low confidence.
| import { | ||
| LOAD_DEFAULT_SITES_SUCCESS, | ||
| LOAD_DEFAULT_SITES_FAILURE, | ||
| SET_DEFAULT_SITES_SUCCESS, | ||
| SET_DEFAULT_SITES_FAILURE, | ||
| DELETE_DEFAULT_SITE_SUCCESS, | ||
| DELETE_DEFAULT_SITE_FAILURE, | ||
| UPDATE_DEFAULT_SITE_SUCCESS, | ||
| UPDATE_DEFAULT_SITE_FAILURE | ||
| } from './actions'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding loading state actions for better UX.
The reducer handles success and failure cases, but missing loading state actions (e.g., LOAD_DEFAULT_SITES_REQUEST) could impact the user experience. Loading states are crucial for showing spinners or disabling buttons during API calls.
Add these action types to handle loading states:
import {
+ LOAD_DEFAULT_SITES_REQUEST,
LOAD_DEFAULT_SITES_SUCCESS,
LOAD_DEFAULT_SITES_FAILURE,
+ SET_DEFAULT_SITES_REQUEST,
SET_DEFAULT_SITES_SUCCESS,
SET_DEFAULT_SITES_FAILURE,
+ DELETE_DEFAULT_SITE_REQUEST,
DELETE_DEFAULT_SITE_SUCCESS,
DELETE_DEFAULT_SITE_FAILURE,
+ UPDATE_DEFAULT_SITE_REQUEST,
UPDATE_DEFAULT_SITE_SUCCESS,
UPDATE_DEFAULT_SITE_FAILURE
} from './actions';📝 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.
| import { | |
| LOAD_DEFAULT_SITES_SUCCESS, | |
| LOAD_DEFAULT_SITES_FAILURE, | |
| SET_DEFAULT_SITES_SUCCESS, | |
| SET_DEFAULT_SITES_FAILURE, | |
| DELETE_DEFAULT_SITE_SUCCESS, | |
| DELETE_DEFAULT_SITE_FAILURE, | |
| UPDATE_DEFAULT_SITE_SUCCESS, | |
| UPDATE_DEFAULT_SITE_FAILURE | |
| } from './actions'; | |
| import { | |
| LOAD_DEFAULT_SITES_REQUEST, | |
| LOAD_DEFAULT_SITES_SUCCESS, | |
| LOAD_DEFAULT_SITES_FAILURE, | |
| SET_DEFAULT_SITES_REQUEST, | |
| SET_DEFAULT_SITES_SUCCESS, | |
| SET_DEFAULT_SITES_FAILURE, | |
| DELETE_DEFAULT_SITE_REQUEST, | |
| DELETE_DEFAULT_SITE_SUCCESS, | |
| DELETE_DEFAULT_SITE_FAILURE, | |
| UPDATE_DEFAULT_SITE_REQUEST, | |
| UPDATE_DEFAULT_SITE_SUCCESS, | |
| UPDATE_DEFAULT_SITE_FAILURE | |
| } from './actions'; |
| export const loadDefaultSitesSuccess = (sites) => ({ | ||
| type: LOAD_DEFAULT_SITES_SUCCESS, | ||
| payload: sites | ||
| }); | ||
|
|
||
| export const loadDefaultSitesFailure = (error) => ({ | ||
| type: LOAD_DEFAULT_SITES_FAILURE, | ||
| payload: error | ||
| }); | ||
|
|
||
| export const setDefaultSitesSuccess = (sites) => ({ | ||
| type: SET_DEFAULT_SITES_SUCCESS, | ||
| payload: sites | ||
| }); | ||
|
|
||
| export const setDefaultSitesFailure = (error) => ({ | ||
| type: SET_DEFAULT_SITES_FAILURE, | ||
| payload: error | ||
| }); | ||
|
|
||
| export const deleteDefaultSiteSuccess = (siteId) => ({ | ||
| type: DELETE_DEFAULT_SITE_SUCCESS, | ||
| payload: siteId | ||
| }); | ||
|
|
||
| export const deleteDefaultSiteFailure = (error) => ({ | ||
| type: DELETE_DEFAULT_SITE_FAILURE, | ||
| payload: error | ||
| }); | ||
|
|
||
| export const updateDefaultSiteSuccess = (site) => ({ | ||
| type: UPDATE_DEFAULT_SITE_SUCCESS, | ||
| payload: site | ||
| }); | ||
|
|
||
| export const updateDefaultSiteFailure = (error) => ({ | ||
| type: UPDATE_DEFAULT_SITE_FAILURE, | ||
| payload: error | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing action creators with type validation and documentation.
The action creators are well-implemented with consistent patterns. Consider these improvements:
- Add runtime type validation for payloads
- Standardize error payload structure
- Add JSDoc comments for better documentation
Example enhancement:
+/**
+ * Action creator for successful default sites loading
+ * @param {Array<Object>} sites - Array of default site objects
+ * @returns {Object} Redux action
+ */
export const loadDefaultSitesSuccess = (sites) => ({
+ if (!Array.isArray(sites)) {
+ throw new TypeError('sites must be an array');
+ }
type: LOAD_DEFAULT_SITES_SUCCESS,
payload: sites
});
+/**
+ * Action creator for default sites loading failure
+ * @param {Error|Object} error - Error object or error response
+ * @returns {Object} Redux action
+ */
export const loadDefaultSitesFailure = (error) => ({
type: LOAD_DEFAULT_SITES_FAILURE,
- payload: error
+ payload: {
+ message: error.message || 'Unknown error occurred',
+ code: error.code,
+ details: error
+ }
});Committable suggestion was skipped due to low confidence.
| export const deleteDefaultSite = (siteId) => async (dispatch) => { | ||
| try { | ||
| await deleteDefaultSelectedSiteApi(siteId); | ||
| dispatch(deleteDefaultSiteSuccess(siteId)); | ||
| return siteId; | ||
| } catch (error) { | ||
| dispatch(deleteDefaultSiteFailure(error)); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the siteId parameter.
Consider adding validation to ensure siteId is provided and has the correct type.
export const deleteDefaultSite = (siteId) => async (dispatch) => {
try {
+ if (!siteId) {
+ throw new Error('Site ID is required');
+ }
await deleteDefaultSelectedSiteApi(siteId);
dispatch(deleteDefaultSiteSuccess(siteId));
return siteId;
} catch (error) {Committable suggestion was skipped due to low confidence.
| export const loadDefaultSites = () => async (dispatch) => { | ||
| try { | ||
| const response = await getDefaultSelectedSitesApi(); | ||
| if (response && response.selected_sites) { | ||
| dispatch(loadDefaultSitesSuccess(response.selected_sites)); | ||
| return response.selected_sites; | ||
| } else { | ||
| dispatch(loadDefaultSitesSuccess([])); | ||
| return []; | ||
| } | ||
| } catch (error) { | ||
| dispatch(loadDefaultSitesFailure(error)); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using optional chaining for safer property access.
The code safely handles both success and empty cases, but we can make it more concise using optional chaining.
- if (response && response.selected_sites) {
+ if (response?.selected_sites) {📝 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.
| export const loadDefaultSites = () => async (dispatch) => { | |
| try { | |
| const response = await getDefaultSelectedSitesApi(); | |
| if (response && response.selected_sites) { | |
| dispatch(loadDefaultSitesSuccess(response.selected_sites)); | |
| return response.selected_sites; | |
| } else { | |
| dispatch(loadDefaultSitesSuccess([])); | |
| return []; | |
| } | |
| } catch (error) { | |
| dispatch(loadDefaultSitesFailure(error)); | |
| throw error; | |
| } | |
| }; | |
| export const loadDefaultSites = () => async (dispatch) => { | |
| try { | |
| const response = await getDefaultSelectedSitesApi(); | |
| if (response?.selected_sites) { | |
| dispatch(loadDefaultSitesSuccess(response.selected_sites)); | |
| return response.selected_sites; | |
| } else { | |
| dispatch(loadDefaultSitesSuccess([])); | |
| return []; | |
| } | |
| } catch (error) { | |
| dispatch(loadDefaultSitesFailure(error)); | |
| throw error; | |
| } | |
| }; |
🧰 Tools
🪛 Biome
[error] 21-21: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| export const setDefaultSites = (sites) => async (dispatch) => { | ||
| try { | ||
| await setDefaultSelectedSitesApi(sites); | ||
| dispatch(setDefaultSitesSuccess(sites)); | ||
| return sites; | ||
| } catch (error) { | ||
| dispatch(setDefaultSitesFailure(error)); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the sites parameter.
While the implementation is clean, it would be beneficial to validate the sites parameter before making the API call.
export const setDefaultSites = (sites) => async (dispatch) => {
try {
+ if (!Array.isArray(sites)) {
+ throw new Error('Sites must be an array');
+ }
await setDefaultSelectedSitesApi(sites);
dispatch(setDefaultSitesSuccess(sites));
return sites;
} catch (error) {📝 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.
| export const setDefaultSites = (sites) => async (dispatch) => { | |
| try { | |
| await setDefaultSelectedSitesApi(sites); | |
| dispatch(setDefaultSitesSuccess(sites)); | |
| return sites; | |
| } catch (error) { | |
| dispatch(setDefaultSitesFailure(error)); | |
| throw error; | |
| } | |
| }; | |
| export const setDefaultSites = (sites) => async (dispatch) => { | |
| try { | |
| if (!Array.isArray(sites)) { | |
| throw new Error('Sites must be an array'); | |
| } | |
| await setDefaultSelectedSitesApi(sites); | |
| dispatch(setDefaultSitesSuccess(sites)); | |
| return sites; | |
| } catch (error) { | |
| dispatch(setDefaultSitesFailure(error)); | |
| throw error; | |
| } | |
| }; |
| const timeoutPromise = new Promise((_, reject) => | ||
| setTimeout(() => reject(new Error('Operation timed out')), SAVE_TIMEOUT) | ||
| ); |
There was a problem hiding this comment.
Add cleanup for timeout promise
The timeout promise should be cleaned up if the component unmounts during the save operation.
useEffect(() => {
+ let timeoutId;
+
const timeoutPromise = new Promise((_, reject) =>
- setTimeout(() => reject(new Error('Operation timed out')), SAVE_TIMEOUT)
+ timeoutId = setTimeout(() => reject(new Error('Operation timed out')), SAVE_TIMEOUT)
);
+
+ return () => {
+ if (timeoutId) clearTimeout(timeoutId);
+ };
}, []);Committable suggestion was skipped due to low confidence.
|
New netmanager changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
Improvements
User Interface