Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analytics: code clean up and optimization #2208

Merged
merged 17 commits into from
Nov 6, 2024
Merged

Analytics: code clean up and optimization #2208

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2879c84
location card functionality update
OchiengPaul442 Oct 29, 2024
989191e
updates to chart loading hook
OchiengPaul442 Oct 29, 2024
c4af9e8
Merge remote-tracking branch 'upstream/staging' into analytics-ui-imp…
OchiengPaul442 Oct 29, 2024
ae005f7
use one source of chart data
OchiengPaul442 Nov 5, 2024
618977b
card checklist order fix
OchiengPaul442 Nov 5, 2024
efe51fb
updated url video link
OchiengPaul442 Nov 5, 2024
046aba0
optimize check list hook
OchiengPaul442 Nov 5, 2024
a784fe7
card design update
OchiengPaul442 Nov 5, 2024
ab52a53
fix calendar issue
OchiengPaul442 Nov 5, 2024
b69a13f
terms of service and policy links added
OchiengPaul442 Nov 5, 2024
0e5c26e
update url link
OchiengPaul442 Nov 5, 2024
5f92e7b
scroll on modal
OchiengPaul442 Nov 5, 2024
b66c1ae
chart optimizations
OchiengPaul442 Nov 6, 2024
7350a00
analytics card optimizations
OchiengPaul442 Nov 6, 2024
4adfadd
use of redux state to fetch and store chart data
OchiengPaul442 Nov 6, 2024
b928601
check list optimization (still work in progress)
OchiengPaul442 Nov 6, 2024
0c782cd
removed other pollutants
OchiengPaul442 Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions platform/src/common/components/Modal/dataDownload/components/DataTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ const TableRowComponent = ({ item, isSelected, onToggleSite, index }) => (
</tr>
);

// Wrap the named function with React.memo and assign displayName
const TableRow = React.memo(TableRowComponent);
TableRow.displayName = 'TableRow';

Expand Down Expand Up @@ -216,7 +215,13 @@ const DataTable = ({

// Handle edge case if filteredData is empty
if (filteredData.length === 0) {
return <p>No data available</p>;
return (
<tr>
<td colSpan="5" className="p-4 text-center text-gray-500">
No data available
</td>
</tr>
);
}

return filteredData
Expand Down
38 changes: 26 additions & 12 deletions platform/src/common/components/Modal/dataDownload/components/LocationCard.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import React from 'react';
import LocationIcon from '@/icons/Analytics/LocationIcon';

// Helper function to handle truncation logic
const truncateName = (name, maxLength = 25) => {
const truncateName = (name, maxLength = 13) => {
if (!name) return 'Unknown Location';
return name.length > maxLength ? `${name.substring(0, maxLength)}...` : name;
};

/**
* LocationCard Component
* Displays information about a location with a checkbox to toggle selection.
*/
const LocationCard = ({ site, onToggle, isSelected, isLoading }) => {
// Display loading skeleton while loading
if (isLoading) {
Expand All @@ -20,33 +25,42 @@ const LocationCard = ({ site, onToggle, isSelected, isLoading }) => {
);
}

// Determine display name (search_name or fallback to location_name)
const displayName = truncateName(
site?.name || site?.search_name?.split(',')[0],
);
// Destructure necessary properties from site
const { name, search_name, country } = site;

// Determine display name (search_name or fallback to name)
const displayName = truncateName(name || search_name?.split(',')[0] || '');
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

LGTM! Consider memoizing the displayName.

The display name logic with fallbacks is well-implemented. However, since truncateName is called on every render, consider memoizing the displayName if the component re-renders frequently.

-  const displayName = truncateName(name || search_name?.split(',')[0] || '');
+  const displayName = React.useMemo(
+    () => truncateName(name || search_name?.split(',')[0] || ''),
+    [name, search_name]
+  );
📝 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.

Suggested change
// Destructure necessary properties from site
const { name, search_name, country } = site;
// Determine display name (search_name or fallback to name)
const displayName = truncateName(name || search_name?.split(',')[0] || '');
// Destructure necessary properties from site
const { name, search_name, country } = site;
// Determine display name (search_name or fallback to name)
const displayName = React.useMemo(
() => truncateName(name || search_name?.split(',')[0] || ''),
[name, search_name]
);


return (
<div
onClick={() => onToggle(site)}
className={`flex justify-between items-center p-3 bg-[#F6F6F7] cursor-pointer border ${
className={`flex justify-between items-center p-3 bg-[#F6F6F7] border ${
isSelected ? 'border-blue-300 ring-2 ring-blue-500' : 'border-gray-200'
} rounded-lg shadow-sm transition-all w-full`}
>
<div className="flex flex-col gap-2">
<h1 className="text-sm font-medium text-gray-900">{displayName}</h1>
<small className="text-xs text-gray-500">
{site.country || 'Unknown Country'}
</small>
<div className="flex items-center gap-2">
<LocationIcon
width={20}
height={20}
fill={isSelected ? '#3B82F6' : '#9EA3AA'}
/>
<div className="flex flex-col">
<h3 className="text-sm font-medium text-gray-900">{displayName}</h3>
<small className="text-xs text-gray-500">
{country || 'Unknown Country'}
</small>
</div>
</div>
<div>
<input
type="checkbox"
id={`checkbox-${site._id || 'unknown'}`}
checked={isSelected}
onChange={(e) => {
e.stopPropagation();
onToggle(site);
}}
className="w-4 h-4 text-blue-600 bg-white cursor-pointer border-blue-300 rounded focus:ring-blue-500"
aria-label={`Select ${displayName}`}
/>
</div>
</div>
Expand Down
53 changes: 37 additions & 16 deletions platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const AddLocations = ({ onClose }) => {

// Local state management
const [selectedSites, setSelectedSites] = useState([]);
const [sidebarSites, setSidebarSites] = useState([]);
const [clearSelected, setClearSelected] = useState(false);
const [error, setError] = useState('');
const [submitLoading, setSubmitLoading] = useState(false);
Expand All @@ -62,13 +63,15 @@ const AddLocations = ({ onClose }) => {

/**
* Populate selectedSites based on selectedSiteIds and fetched sitesSummaryData.
* Also initializes sidebarSites with the initially selected sites.
*/
useEffect(() => {
if (sitesSummaryData && selectedSiteIds.length) {
const initialSelectedSites = sitesSummaryData.filter((site) =>
selectedSiteIds.includes(site._id),
);
setSelectedSites(initialSelectedSites);
setSidebarSites(initialSelectedSites);
}
}, [sitesSummaryData, selectedSiteIds]);

Expand All @@ -84,16 +87,32 @@ const AddLocations = ({ onClose }) => {

/**
* Toggles the selection of a site.
* @param {Object} site - The site to toggle.
* Adds to selectedSites and sidebarSites if selected.
* Removes from selectedSites but retains in sidebarSites if unselected.
*/
const handleToggleSite = useCallback((site) => {
setSelectedSites((prev) => {
const isSelected = prev.some((s) => s._id === site._id);
return isSelected
? prev.filter((s) => s._id !== site._id)
: [...prev, site];
});
}, []);
const handleToggleSite = useCallback(
(site) => {
setSelectedSites((prev) => {
const isSelected = prev.some((s) => s._id === site._id);
if (isSelected) {
// Remove from selectedSites
return prev.filter((s) => s._id !== site._id);
} else {
// Add to selectedSites
return [...prev, site];
}
});

setSidebarSites((prev) => {
const alreadyInSidebar = prev.some((s) => s._id === site._id);
if (!alreadyInSidebar) {
return [...prev, site];
}
return prev;
});
},
[setSelectedSites, setSidebarSites],
);
Comment on lines +93 to +115
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider preventing selection of more than 4 locations.

Currently, the 4-location limit is only enforced during submission. Consider preventing users from selecting more than 4 locations in the first place to provide better user feedback.

Update the handleToggleSite function to include this check:

 const handleToggleSite = useCallback(
   (site) => {
+    setSelectedSites((prev) => {
+      const isSelected = prev.some((s) => s._id === site._id);
+      if (isSelected) {
+        return prev.filter((s) => s._id !== site._id);
+      } else if (prev.length >= 4) {
+        setError('You can select up to 4 locations only');
+        return prev;
+      }
+      return [...prev, site];
+    });
     
     setSidebarSites((prev) => 
       prev.some((s) => s._id === site._id) ? prev : [...prev, site]
     );
   },
-  [setSelectedSites, setSidebarSites],
+  [setSelectedSites, setSidebarSites, setError],
 );

Also applies to: 132-139

Comment on lines +90 to +115
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider optimizing state updates.

The function correctly manages both states, but we can optimize it by combining the state updates and reducing complexity.

Consider this more efficient implementation:

 const handleToggleSite = useCallback(
   (site) => {
-    setSelectedSites((prev) => {
-      const isSelected = prev.some((s) => s._id === site._id);
-      if (isSelected) {
-        return prev.filter((s) => s._id !== site._id);
-      } else {
-        return [...prev, site];
-      }
-    });
-
-    setSidebarSites((prev) => {
-      const alreadyInSidebar = prev.some((s) => s._id === site._id);
-      if (!alreadyInSidebar) {
-        return [...prev, site];
-      }
-      return prev;
-    });
+    setSelectedSites((prev) => 
+      prev.some((s) => s._id === site._id)
+        ? prev.filter((s) => s._id !== site._id)
+        : [...prev, site]
+    );
+    setSidebarSites((prev) => 
+      prev.some((s) => s._id === site._id) ? prev : [...prev, site]
+    );
   },
   [setSelectedSites, setSidebarSites],
 );
📝 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.

Suggested change
* Adds to selectedSites and sidebarSites if selected.
* Removes from selectedSites but retains in sidebarSites if unselected.
*/
const handleToggleSite = useCallback((site) => {
setSelectedSites((prev) => {
const isSelected = prev.some((s) => s._id === site._id);
return isSelected
? prev.filter((s) => s._id !== site._id)
: [...prev, site];
});
}, []);
const handleToggleSite = useCallback(
(site) => {
setSelectedSites((prev) => {
const isSelected = prev.some((s) => s._id === site._id);
if (isSelected) {
// Remove from selectedSites
return prev.filter((s) => s._id !== site._id);
} else {
// Add to selectedSites
return [...prev, site];
}
});
setSidebarSites((prev) => {
const alreadyInSidebar = prev.some((s) => s._id === site._id);
if (!alreadyInSidebar) {
return [...prev, site];
}
return prev;
});
},
[setSelectedSites, setSidebarSites],
);
* Adds to selectedSites and sidebarSites if selected.
* Removes from selectedSites but retains in sidebarSites if unselected.
*/
const handleToggleSite = useCallback(
(site) => {
setSelectedSites((prev) =>
prev.some((s) => s._id === site._id)
? prev.filter((s) => s._id !== site._id)
: [...prev, site]
);
setSidebarSites((prev) =>
prev.some((s) => s._id === site._id) ? prev : [...prev, site]
);
},
[setSelectedSites, setSidebarSites],
);


/**
* Handles the submission of selected sites.
Expand Down Expand Up @@ -149,9 +168,11 @@ const AddLocations = ({ onClose }) => {
}, [selectedSites, userID, dispatch, onClose]);

/**
* Generates the content for the selected sites panel.
* Generates the content for the sidebar.
* Displays only the sites that have been selected at least once.
* Each card reflects its current selection status.
*/
const selectedSitesContent = useMemo(() => {
const sidebarSitesContent = useMemo(() => {
if (loading) {
return (
<div className="space-y-4">
Expand All @@ -168,7 +189,7 @@ const AddLocations = ({ onClose }) => {
);
}

if (selectedSites.length === 0) {
if (sidebarSites.length === 0) {
return (
<div className="text-gray-500 w-full text-sm h-full flex flex-col justify-center items-center">
<span className="p-2 rounded-full bg-[#F6F6F7] mb-2">
Expand All @@ -179,22 +200,22 @@ const AddLocations = ({ onClose }) => {
);
}

return selectedSites.map((site) => (
return sidebarSites.map((site) => (
<LocationCard
key={site._id}
site={site}
onToggle={handleToggleSite}
isLoading={loading}
isSelected={true}
isSelected={selectedSites.some((s) => s._id === site._id)}
/>
));
}, [selectedSites, handleToggleSite, loading]);
}, [sidebarSites, selectedSites, handleToggleSite, loading]);

return (
<>
{/* Selected Sites Sidebar */}
<div className="w-[280px] h-[658px] overflow-y-auto border-r relative space-y-3 px-4 pt-5 pb-14">
{selectedSitesContent}
{sidebarSitesContent}
</div>

{/* Main Content Area */}
Expand Down
7 changes: 0 additions & 7 deletions platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,6 @@ const MoreInsights = () => {
{/* -------------------- Main Content Area -------------------- */}
<div className="bg-white relative w-full h-full">
<div className="px-8 pt-6 pb-4 space-y-4 relative h-full overflow-hidden">
{/* -------------------- Error Display -------------------- */}
{error && (
<div className="w-full p-4 bg-red-100 text-red-700 rounded-md">
{error}
</div>
)}

{/* -------------------- Controls: Dropdowns and Actions -------------------- */}
<div className="w-full flex flex-wrap gap-2 justify-between">
<div className="space-x-2 flex">
Expand Down
2 changes: 1 addition & 1 deletion platform/src/core/urls/analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ export const DATA_EXPORT_URL = `${BASE_AUTH_URL}/analytics/data-download`;

export const SHARE_REPORT_URL = `${BASE_AUTH_URL}/users/emailReport`;

export const SITES_SUMMARY_URL = `${BASE_AUTH_URL}/devices/sites/summary?online_status=online`;
export const SITES_SUMMARY_URL = `${BASE_AUTH_URL}/devices/sites/summary`;
13 changes: 6 additions & 7 deletions platform/src/core/utils/useFetchAnalyticsData.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@ const useFetchAnalyticsData = ({
setError(null);

try {
if (selectedSiteIds.length === 0) {
setAllSiteData([]);
setError(null);
setChartLoading(false);
return;
}
// if (selectedSiteIds.length === 0) {
// setAllSiteData([]);
// setError(null);
// return;
// }
Comment on lines +28 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Reconsider removing the early return for empty site selections.

The removal of this guard clause means we'll make API calls even when no sites are selected. This could lead to unnecessary network requests and potential backend issues if the API doesn't handle empty site arrays gracefully.

Consider restoring the early return with a slight modification:

-      // if (selectedSiteIds.length === 0) {
-      //   setAllSiteData([]);
-      //   setError(null);
-      //   return;
-      // }
+      if (selectedSiteIds.length === 0) {
+        setAllSiteData([]);
+        setError(null);
+        setChartLoading(false);
+        return;
+      }
📝 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.

Suggested change
// if (selectedSiteIds.length === 0) {
// setAllSiteData([]);
// setError(null);
// return;
// }
if (selectedSiteIds.length === 0) {
setAllSiteData([]);
setError(null);
setChartLoading(false);
return;
}


const requestBody = {
sites: selectedSiteIds,
Expand All @@ -51,6 +50,7 @@ const useFetchAnalyticsData = ({

if (response.status === 'success' && Array.isArray(response.data)) {
setAllSiteData(response.data);
setChartLoading(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance loading state management and add cleanup.

The loading state management could be improved to handle all scenarios, including request cancellation. Additionally, the useEffect cleanup is missing for the abort controller.

Consider implementing these improvements:

 const fetchAnalyticsData = useCallback(async () => {
     setChartLoading(true);
     setError(null);
+    const controller = new AbortController();
 
     try {
-      const controller = new AbortController();
       // ... existing code ...
     } catch (err) {
+      if (err.name === 'AbortError') {
+        return;
+      }
       console.error('Error fetching analytics data:', err);
       setError(err.message || 'An unexpected error occurred.');
       setAllSiteData([]);
+    } finally {
       setChartLoading(false);
     }
+    return () => controller.abort();
 }, [/* ... */]);

 useEffect(() => {
-    fetchAnalyticsData();
+    const cleanup = fetchAnalyticsData();
+    return () => cleanup?.();
 }, [fetchAnalyticsData]);

Also applies to: 74-76

} else {
setAllSiteData([]);
throw new Error(response.message || 'Failed to fetch analytics data.');
Expand All @@ -59,7 +59,6 @@ const useFetchAnalyticsData = ({
console.error('Error fetching analytics data:', err);
setError(err.message || 'An unexpected error occurred.');
setAllSiteData([]);
} finally {
setChartLoading(false);
}
}, [
Expand Down
Loading