Analytics: Enhancements on More insights modal #2282
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 5583163 | Triggered | Google API Key | 620249e | mobile-v3/ios/Runner/AppDelegate.swift | View secret |
| 5583163 | Triggered | Google API Key | 620249e | mobile-v3/android/app/src/main/AndroidManifest.xml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements across multiple components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
platform/src/common/components/Modal/dataDownload/components/LocationCard.jsx (2)
39-41: Consider improving the display name logic.The current nested fallback logic could be more readable using optional chaining and nullish coalescing.
Here's a cleaner approach:
- const displayName = truncateName( - name || (search_name && search_name.split(',')[0]) || '', - ); + const displayName = truncateName( + name ?? search_name?.split(',')[0] ?? '' + );🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
85-101: Consider enhancing PropTypes validation.While the current validation is good, we could make it more specific for better type safety.
Consider adding these validations:
LocationCard.propTypes = { site: PropTypes.shape({ _id: PropTypes.string.isRequired, name: PropTypes.string, search_name: PropTypes.string, country: PropTypes.string, + // Add validation for any other potential site properties }).isRequired, onToggle: PropTypes.func.isRequired, - isSelected: PropTypes.bool.isRequired, + isSelected: PropTypes.bool.isRequired, isLoading: PropTypes.bool, - disableToggle: PropTypes.bool, // New prop to disable toggle when needed + disableToggle: PropTypes.bool, };platform/src/common/components/Charts/components/index.jsx (1)
253-259: Clean and efficient legend rendering!The conditional rendering logic for truncated legends is well-implemented. However, consider adding a tooltip placement prop for better positioning control.
- <Tooltip content={entry.value} className="w-auto"> + <Tooltip content={entry.value} className="w-auto" placement="top">platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
211-258: Download functionality needs retry mechanismThe download functionality is well-implemented with proper error handling, but consider adding a retry mechanism for failed downloads.
const handleDataDownload = async () => { setDownloadLoading(true); + let retryCount = 0; + const maxRetries = 3; + + const attemptDownload = async () => { try { const { startDate, endDate } = dateRange; // ... existing code ... const response = await exportDataApi(apiData); // ... existing code ... setDownloadLoading(false); CustomToast(); } catch (error) { console.error(error); + if (retryCount < maxRetries) { + retryCount++; + console.log(`Retrying download (${retryCount}/${maxRetries})...`); + await attemptDownload(); + } else { setDownloadError( 'There was an error downloading the data. Please try again later.', ); setDownloadLoading(false); + } } + }; + + await attemptDownload(); };
417-422: Consider enhancing AirQualityCard with real-time dataThe AirQualityCard is currently using placeholder values. Consider enhancing it with real-time data from the chart.
<AirQualityCard - airQuality="--" - pollutionSource="--" + airQuality={allSiteData?.length > 0 ? allSiteData[0].airQuality : '--'} + pollutionSource={allSiteData?.length > 0 ? allSiteData[0].source : '--'} pollutant={chartData.pollutionType === 'pm2_5' ? 'PM2.5' : 'PM10'} isLoading={chartLoading} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
platform/src/common/components/Button/TabButtons.jsx(2 hunks)platform/src/common/components/Charts/components/index.jsx(1 hunks)platform/src/common/components/Modal/dataDownload/components/LocationCard.jsx(4 hunks)platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx(10 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
platform/src/common/components/Modal/dataDownload/components/LocationCard.jsx
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
platform/src/common/components/Modal/dataDownload/components/LocationCard.jsx (3)
15-21: LGTM! Clean prop destructuring and naming.
The component signature is well-structured with clear, purpose-indicating prop names.
47-50: Great job on accessibility and visual feedback!
The styling properly reflects the component's state, and the aria-disabled attribute enhances accessibility.
72-74: Well-implemented toggle functionality.
Good practices observed:
- Event propagation control with
stopPropagation - Clean conditional toggle logic
- Proper disabled state handling on the checkbox
Also applies to: 78-78
platform/src/common/components/Button/TabButtons.jsx (4)
4-4: Documentation and imports look good!
The component is well-documented with JSDoc comments, clearly describing its purpose and props.
Also applies to: 6-14
34-40: Great accessibility implementation!
The button's disabled state is properly handled with both disabled and aria-disabled attributes, and the cursor style changes appropriately.
42-51: Clean loading state implementation!
The loading state is well-implemented with a conditional render between Spinner and Icon.
72-84: PropTypes are well-defined!
The PropTypes are comprehensive and properly typed, especially the Icon prop which now accepts both function and element types.
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
47-49: Well-structured state management!
The download-related states are properly initialized and follow React best practices.
368-375: Download button implementation looks good!
The download button properly handles loading and disabled states using the TabButtons component.
| /** | ||
| * Ensure `allSites` is an array. | ||
| * If `modalData` is undefined, null, or not an array, default to an empty array. | ||
| */ | ||
| const allSites = useMemo(() => { | ||
| if (Array.isArray(modalData)) return modalData; | ||
| if (modalData) return [modalData]; | ||
| return []; | ||
| }, [modalData]); | ||
|
|
||
| // Extract site IDs from selected sites for API requests | ||
| /** | ||
| * Local state to manage selected sites. | ||
| * Initializes with all sites selected by default. | ||
| */ | ||
| const [selectedSites, setSelectedSites] = useState(() => [...allSites]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Site selection logic needs error handling enhancement
The site selection logic is well-implemented, but consider adding error handling for edge cases.
const toggleSiteSelection = useCallback(
(siteId) => {
+ if (!siteId) {
+ console.error('Invalid site ID provided');
+ return;
+ }
setSelectedSites((prevSelected) => {
const isSelected = prevSelected.some((site) => site._id === siteId);
if (isSelected) {
return prevSelected.filter((site) => site._id !== siteId);
} else {
const siteToAdd = allSites.find((site) => site._id === siteId);
if (siteToAdd) {
return [...prevSelected, siteToAdd];
}
+ console.error(`Site with ID ${siteId} not found`);
return prevSelected;
}
});
},
[allSites],
);Also applies to: 120-141
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx(10 hunks)
🔇 Additional comments (1)
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (1)
126-141: Site selection logic needs error handling for edge cases
Consider adding error handling for invalid or non-existent site IDs.
As previously suggested, it's important to handle cases where siteId might be invalid or not found in allSites. This prevents potential runtime errors and improves the robustness of your code.
Apply this diff to enhance error handling:
const toggleSiteSelection = useCallback(
(siteId) => {
+ if (!siteId) {
+ console.error('Invalid site ID provided');
+ return;
+ }
setSelectedSites((prevSelected) => {
const isSelected = prevSelected.some((site) => site._id === siteId);
if (isSelected) {
return prevSelected.filter((site) => site._id !== siteId);
} else {
const siteToAdd = allSites.find((site) => site._id === siteId);
if (siteToAdd) {
return [...prevSelected, siteToAdd];
}
+ console.error(`Site with ID ${siteId} not found`);
return prevSelected;
}
});
},
[allSites],
);| startDateTime: startDate, | ||
| endDateTime: endDate, | ||
| sites: selectedSiteIds, | ||
| network: chartData.organizationName, |
There was a problem hiding this comment.
Inconsistent property naming: 'organisationName' vs 'organizationName'
In your API request data, you're using chartData.organizationName for the network parameter, but elsewhere you reference chartData.organisationName. This inconsistency might lead to undefined values and potential errors. Please ensure consistent property naming throughout your code.
There was a problem hiding this comment.
⚠️ Potential issueInconsistent property naming: 'organisationName' vs 'organizationName'
In your API request data, you're using
chartData.organizationNamefor thenetworkparameter, but elsewhere you referencechartData.organisationName. This inconsistency might lead toundefinedvalues and potential errors. Please ensure consistent property naming throughout your code.
Hi @OchiengPaul442 , can we quickly start using GROUPS in new Analytics and quickly phase out "organisations" and "networks" from the new Analytics codebase? The earlier we are intentional about the entire process, the better for all of us. cc: @Codebmk
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
New next-platform changes available for preview here |
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
TabButtonscomponent with loading and disabled states, including a loading spinner for better user feedback.disableToggleprop toLocationCardfor optional toggle functionality.MoreInsightswith error handling and user notifications.Improvements
MoreInsightscomponent.These updates enhance usability and accessibility across the application.