Feature/intervention #2343
Feature/intervention #2343
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shyambhongle
force-pushed
the
feature/intervention
branch
from
53257dc to
43d3856
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shyambhongle
force-pushed
the
feature/intervention
branch
from
5fb466c to
9f9021c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/features/projectsV2/ProjectDetails/index.tsx (2)
131-134: 🛠️ Refactor suggestionMove isNonPlantationType check to utils file.
The
shouldShowOtherInterventionlogic should be moved to a utility function for better code organization.Create a new utility function in the utils file:
// utils/projectV2.ts export const shouldShowOtherIntervention = ( hoveredLocation: PlantLocation | null, selectedLocation: PlantLocation | null ): boolean => { return isNonPlantationType(hoveredLocation) || isNonPlantationType(selectedLocation); };
195-204: 🛠️ Refactor suggestionSimplify complex conditional prop assignment.
The prop assignments for
OtherInterventionInfoare complex and hard to read. Consider extracting the logic into a helper function.+const getInterventionLocationInfo = ( + location: PlantLocation | null, +): PlantLocation | null => { + if (!location) return null; + return location.type !== 'single-tree-registration' && + location.type !== 'multi-tree-registration' + ? location + : null; +}; {shouldShowOtherIntervention ? ( <OtherInterventionInfo - selectedPlantLocation={selectedPlantLocation && selectedPlantLocation?.type !== 'single-tree-registration' && - selectedPlantLocation?.type !== 'multi-tree-registration' ? selectedPlantLocation : null} - hoveredPlantLocation={hoveredPlantLocation && hoveredPlantLocation?.type !== 'single-tree-registration' && - hoveredPlantLocation?.type !== 'multi-tree-registration' ? hoveredPlantLocation : null} + selectedPlantLocation={getInterventionLocationInfo(selectedPlantLocation)} + hoveredPlantLocation={getInterventionLocationInfo(hoveredPlantLocation)} setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} isMobile={isMobile} /> ) : null}
🧹 Nitpick comments (2)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (2)
117-128: Optimize thetotalTreesCountcalculation.Simplify the logic using optional chaining and remove unnecessary dependency.
const { totalTreesCount } = useMemo(() => { const totalTreesCount = - plantLocationInfo && - plantLocationInfo.plantedSpecies && - plantLocationInfo.plantedSpecies.length > 0 + plantLocationInfo?.plantedSpecies?.length > 0 ? plantLocationInfo.plantedSpecies.reduce( (sum, species: { treeCount: number }) => sum + species.treeCount, 0 ) : 0; return { totalTreesCount }; - }, [plantLocationInfo, plantLocationInfo?.type]); + }, [plantLocationInfo?.plantedSpecies]);🧰 Tools
🪛 Biome (1.9.4)
[error] 119-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
130-143: Simplify thesampleInterventionSpeciesImageslogic.The current implementation has unnecessary complexity and potential undefined access.
const sampleInterventionSpeciesImages = useMemo(() => { - if (plantLocationInfo && plantLocationInfo.sampleInterventions.length > 0) { - const result = - plantLocationInfo.sampleInterventions && - plantLocationInfo.sampleInterventions.map((item) => { + if (plantLocationInfo?.sampleInterventions?.length > 0) { + return plantLocationInfo.sampleInterventions.map((item) => { return { id: item.coordinates[0].id, - image: item.coordinates[0].image ?? '', + image: item.coordinates[0]?.image ?? '', description: tProjectDetails('sampleTreeTag', { tag: item.tag }), }; }); - return result; } + return []; }, [plantLocationInfo]);🧰 Tools
🪛 Biome (1.9.4)
[error] 133-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/static/locales/en/projectDetails.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx(1 hunks)src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectDetails/index.tsx(3 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx(1 hunks)src/features/projectsV2/ProjectsMap/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/projectsV2/ProjectsMap/index.tsx
🧰 Additional context used
🪛 GitHub Check: CodeFactor
src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'AllInterventions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 14-62: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L14-L62
Complex Method
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L2
'INTERVENTION_TYPE' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx
[error] 119-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 133-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 185-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx (1)
20-21: Update class names to match component purpose.The class names still reference plant location and planting details, which don't align with the intervention context.
- className={`plant-location-header-container ${styles.plantLocationHeaderContainer}`} + className={`intervention-header-container ${styles.interventionHeaderContainer}`} - className={`planting-details-item ${styles.plantingDetailsItem}`} + className={`intervention-details-item ${styles.interventionDetailsItem}`}Also applies to: 26-27
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx (2)
24-27: Rename handler function to match its purpose.The function name
handleFilterSelectionshould be renamed to better reflect its purpose of handling intervention selection.- const handleFilterSelection = (key: INTERVENTION_TYPE) => { + const handleInterventionSelection = (key: INTERVENTION_TYPE) => { setIsMenuOpen(false); setSelectedInterventionType(key); };
30-47: Improve accessibility and list rendering.The list implementation needs improvements for accessibility and React best practices:
- Using index as key in map function
- Missing accessibility attributes
- Missing keyboard navigation
- <ul className={styles.interventionListOptions}> + <ul className={styles.interventionListOptions} role="listbox"> {interventionList.map((intervention, index) => { return ( <li className={`${styles.listItem} ${intervention.value === selectedInterventionData?.value ? styles.selectedItem : ''}`} - onClick={() => handleFilterSelection(intervention.value)} - key={index} + onClick={() => handleInterventionSelection(intervention.value)} + key={intervention.value} + role="option" + aria-selected={intervention.value === selectedInterventionData?.value} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleInterventionSelection(intervention.value); + } + }} > <p> <OtherInterventionTitle type={intervention.value} /> </p> </li> ); })} </ul>src/features/projectsV2/ProjectDetails/components/OtherInterventionInfo.tsx (1)
196-208: Address mobile UI layout issues.Based on past review comments, there are issues with the mobile layout where dropdowns overlap and heights need adjustment.
Please verify the mobile layout by:
- Testing different screen sizes
- Ensuring dropdowns don't overlap
- Adjusting container heights appropriately
Consider adding responsive design utilities or CSS variables to manage component heights consistently across different views.
| import React from "react"; | ||
| import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention"; | ||
| import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention"; | ||
| import { useTranslations } from "next-intl"; |
There was a problem hiding this comment.
Remove unused imports to improve code maintainability.
The following imports are defined but never used:
INTERVENTION_TYPEAllInterventionsfindInterventionHeader
import React from "react";
-import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention";
-import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention";
import { useTranslations } from "next-intl";📝 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 from "react"; | |
| import type { INTERVENTION_TYPE } from "../../../../utils/constants/intervention"; | |
| import { AllInterventions, findInterventionHeader } from "../../../../utils/constants/intervention"; | |
| import { useTranslations } from "next-intl"; | |
| import React from "react"; | |
| import { useTranslations } from "next-intl"; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'AllInterventions' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
[notice] 2-2: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L2
'INTERVENTION_TYPE' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
| default: | ||
| '' | ||
| break; | ||
| } |
There was a problem hiding this comment.
Fix the default case in switch statement.
The default case has an unreachable break statement after an empty string literal.
default:
- ''
- break;
+ 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.
| default: | |
| '' | |
| break; | |
| } | |
| default: | |
| return ''; | |
| } |
| const selectDynamicString = () => { | ||
| switch (type) { | ||
| case 'all': | ||
| return tProjectDetails('all'); | ||
| case 'default': | ||
| return tProjectDetails('default'); | ||
| case 'single-tree-registration': | ||
| return tProjectDetails('single-tree-registration'); | ||
| case 'multi-tree-registration': | ||
| return tProjectDetails('multi-tree-registration'); | ||
| case 'fire-suppression': | ||
| return tProjectDetails('fire-suppression'); | ||
| case 'soil-improvement': | ||
| return tProjectDetails('soil-improvement'); | ||
| case 'stop-tree-harvesting': | ||
| return tProjectDetails('stop-tree-harvesting'); | ||
| case 'removal-invasive-species': | ||
| return tProjectDetails('removal-invasive-species'); | ||
| case 'assisting-seed-rain': | ||
| return tProjectDetails('assisting-seed-rain'); | ||
| case 'fencing': | ||
| return tProjectDetails('fencing'); | ||
| case 'grass-suppression': | ||
| return tProjectDetails('grass-suppression'); | ||
| case 'direct-seeding': | ||
| return tProjectDetails('direct-seeding'); | ||
| case 'enrichment-planting': | ||
| return tProjectDetails('enrichment-planting'); | ||
| case 'firebreaks': | ||
| return tProjectDetails('firebreaks'); | ||
| case 'fire-patrol': | ||
| return tProjectDetails('fire-patrol'); | ||
| case 'liberating-regenerant': | ||
| return tProjectDetails('liberating-regenerant'); | ||
| case 'maintenance': | ||
| return tProjectDetails('maintenance'); | ||
| case 'marking-regenerant': | ||
| return tProjectDetails('marking-regenerant'); | ||
| case 'other-intervention': | ||
| return tProjectDetails('other-intervention'); | ||
| default: | ||
| '' | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the switch statement to reduce complexity.
The selectDynamicString function contains a large switch statement that could be simplified using a mapping object.
const OtherInterventionTitle = ({ type }: Props) => {
const tProjectDetails = useTranslations('ProjectDetails.intervention');
- const selectDynamicString = () => {
- switch (type) {
- case 'all':
- return tProjectDetails('all');
- case 'default':
- return tProjectDetails('default');
- // ... more cases
- default:
- ''
- break;
- }
- }
+ const interventionTypes = {
+ 'all': 'all',
+ 'default': 'default',
+ 'single-tree-registration': 'single-tree-registration',
+ 'multi-tree-registration': 'multi-tree-registration',
+ 'fire-suppression': 'fire-suppression',
+ 'soil-improvement': 'soil-improvement',
+ 'stop-tree-harvesting': 'stop-tree-harvesting',
+ 'removal-invasive-species': 'removal-invasive-species',
+ 'assisting-seed-rain': 'assisting-seed-rain',
+ 'fencing': 'fencing',
+ 'grass-suppression': 'grass-suppression',
+ 'direct-seeding': 'direct-seeding',
+ 'enrichment-planting': 'enrichment-planting',
+ 'firebreaks': 'firebreaks',
+ 'fire-patrol': 'fire-patrol',
+ 'liberating-regenerant': 'liberating-regenerant',
+ 'maintenance': 'maintenance',
+ 'marking-regenerant': 'marking-regenerant',
+ 'other-intervention': 'other-intervention'
+ } as const;
+ const selectDynamicString = () => type in interventionTypes ? tProjectDetails(interventionTypes[type as keyof typeof interventionTypes]) : '';📝 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 selectDynamicString = () => { | |
| switch (type) { | |
| case 'all': | |
| return tProjectDetails('all'); | |
| case 'default': | |
| return tProjectDetails('default'); | |
| case 'single-tree-registration': | |
| return tProjectDetails('single-tree-registration'); | |
| case 'multi-tree-registration': | |
| return tProjectDetails('multi-tree-registration'); | |
| case 'fire-suppression': | |
| return tProjectDetails('fire-suppression'); | |
| case 'soil-improvement': | |
| return tProjectDetails('soil-improvement'); | |
| case 'stop-tree-harvesting': | |
| return tProjectDetails('stop-tree-harvesting'); | |
| case 'removal-invasive-species': | |
| return tProjectDetails('removal-invasive-species'); | |
| case 'assisting-seed-rain': | |
| return tProjectDetails('assisting-seed-rain'); | |
| case 'fencing': | |
| return tProjectDetails('fencing'); | |
| case 'grass-suppression': | |
| return tProjectDetails('grass-suppression'); | |
| case 'direct-seeding': | |
| return tProjectDetails('direct-seeding'); | |
| case 'enrichment-planting': | |
| return tProjectDetails('enrichment-planting'); | |
| case 'firebreaks': | |
| return tProjectDetails('firebreaks'); | |
| case 'fire-patrol': | |
| return tProjectDetails('fire-patrol'); | |
| case 'liberating-regenerant': | |
| return tProjectDetails('liberating-regenerant'); | |
| case 'maintenance': | |
| return tProjectDetails('maintenance'); | |
| case 'marking-regenerant': | |
| return tProjectDetails('marking-regenerant'); | |
| case 'other-intervention': | |
| return tProjectDetails('other-intervention'); | |
| default: | |
| '' | |
| break; | |
| } | |
| } | |
| const interventionTypes = { | |
| 'all': 'all', | |
| 'default': 'default', | |
| 'single-tree-registration': 'single-tree-registration', | |
| 'multi-tree-registration': 'multi-tree-registration', | |
| 'fire-suppression': 'fire-suppression', | |
| 'soil-improvement': 'soil-improvement', | |
| 'stop-tree-harvesting': 'stop-tree-harvesting', | |
| 'removal-invasive-species': 'removal-invasive-species', | |
| 'assisting-seed-rain': 'assisting-seed-rain', | |
| 'fencing': 'fencing', | |
| 'grass-suppression': 'grass-suppression', | |
| 'direct-seeding': 'direct-seeding', | |
| 'enrichment-planting': 'enrichment-planting', | |
| 'firebreaks': 'firebreaks', | |
| 'fire-patrol': 'fire-patrol', | |
| 'liberating-regenerant': 'liberating-regenerant', | |
| 'maintenance': 'maintenance', | |
| 'marking-regenerant': 'marking-regenerant', | |
| 'other-intervention': 'other-intervention' | |
| } as const; | |
| const selectDynamicString = () => type in interventionTypes ? tProjectDetails(interventionTypes[type as keyof typeof interventionTypes]) : ''; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 14-62: src/features/projectsV2/ProjectDetails/components/OtherInterventionTitle.tsx#L14-L62
Complex Method
| import styles from '../../styles/PlantLocationInfo.module.scss'; | ||
| import { formatHid } from '../../../../../utils/projectV2'; | ||
| import { findInterventionHeader } from '../../../../../utils/constants/intervention'; |
There was a problem hiding this comment.
Remove unused import and update import path.
The findInterventionHeader import is not used in the component.
import styles from '../../styles/PlantLocationInfo.module.scss';
import { formatHid } from '../../../../../utils/projectV2';
-import { findInterventionHeader } from '../../../../../utils/constants/intervention';📝 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 styles from '../../styles/PlantLocationInfo.module.scss'; | |
| import { formatHid } from '../../../../../utils/projectV2'; | |
| import { findInterventionHeader } from '../../../../../utils/constants/intervention'; | |
| import styles from '../../styles/PlantLocationInfo.module.scss'; | |
| import { formatHid } from '../../../../../utils/projectV2'; |
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 3-3: src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx#L3
'findInterventionHeader' is defined but never used. Allowed unused vars must match /^_/u. (@typescript-eslint/no-unused-vars)
| const createCardData = (plantLocationInfo: OtherInterventions | null) => { | ||
| // Initialize an array to store the cleaned key-value pairs | ||
| const cleanedData: { key: string; value: string }[] = []; | ||
|
|
||
| // Extract metadata from the plantLocationInfo object, if it exists | ||
| const parsedData = plantLocationInfo?.metadata; | ||
|
|
||
| // Check if `parsedData.public` exists, is an object, and is not an array | ||
| if ( | ||
| parsedData?.public && | ||
| typeof parsedData.public === 'object' && | ||
| !Array.isArray(parsedData.public) | ||
| ) { | ||
| // Iterate over the entries of `parsedData.public` as key-value pairs | ||
| Object.entries(parsedData.public as PublicMetaData).forEach( | ||
| ([key, value]) => { | ||
| // Skip the entry if the key is 'isEntireSite' as it's used to show point location and no use to user | ||
| if (key !== 'isEntireSite') { | ||
| // If the value is a string, directly add it to cleanedData | ||
| if (typeof value === 'string') { | ||
| cleanedData.push({ value, key }); | ||
| } | ||
| // If the value is an object with `value` and `label` properties | ||
| else if ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| 'value' in value && | ||
| 'label' in value | ||
| ) { | ||
| // Check if the `value` property contains a valid JSON string | ||
| if (isJsonString(value.value)) { | ||
| try { | ||
| // Parse the JSON string | ||
| const parsedValue = JSON.parse(value.value); | ||
| // If the parsed value is an object with a `value` property, add it to cleanedData | ||
| if ( | ||
| parsedValue && | ||
| typeof parsedValue === 'object' && | ||
| 'value' in parsedValue | ||
| ) { | ||
| cleanedData.push({ | ||
| key: value.label, // Use the `label` property as the key | ||
| value: parsedValue.value, // Use the parsed `value` property | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| // Log an error if JSON parsing fails | ||
| console.error('Error parsing JSON:', error); | ||
| } | ||
| } else { | ||
| // If not a JSON string, add the `label` and `value` directly | ||
| cleanedData.push({ | ||
| key: value.label, | ||
| value: value.value, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Return the array of cleaned key-value pairs | ||
| return cleanedData; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move createCardData to a dedicated utility file.
For better maintainability and reusability, move this function to a separate utility file. This aligns with the single responsibility principle and makes the code more modular.
Create a new file src/features/projectsV2/ProjectDetails/utils/metadata.ts:
import { OtherInterventions } from '../../../common/types/plantLocation';
interface MetaDataValue {
value: string;
label: string;
}
interface PublicMetaData {
[key: string]: string | MetaDataValue;
}
/**
* Processes and cleans metadata from plant location information
* @param plantLocationInfo - The plant location information containing metadata
* @returns Array of cleaned key-value pairs with metadata
*/
export function cleanPublicMetadata(plantLocationInfo: OtherInterventions | null) {
// Move existing implementation here...
}| const content = [ | ||
| <> | ||
| <InterventionHeader | ||
| plHid={plantLocationInfo?.hid} | ||
| interventionType={plantLocationInfo?.type} | ||
| plantDate={plantLocationInfo?.interventionStartDate} | ||
| key="interventionHeader" | ||
| /> | ||
| {shouldDisplayImageCarousel && ( | ||
| <ImageSlider | ||
| key="imageSlider" | ||
| images={sampleInterventionSpeciesImages} | ||
| type="coordinate" | ||
| isMobile={isMobile} | ||
| imageSize="large" | ||
| allowFullView={!isMobile} | ||
| /> | ||
| )} | ||
| </>, | ||
| cleanedPublicMetadata.length > 0 && ( | ||
| <OtherInterventionMetadata | ||
| key="plantingDetails" | ||
| metadata={cleanedPublicMetadata} | ||
| plantDate={plantLocationInfo?.interventionStartDate} | ||
| type={plantLocationInfo?.type} | ||
| />), | ||
| plantLocationInfo?.plantedSpecies && | ||
| plantLocationInfo.plantedSpecies.length > 0 && ( | ||
| <SpeciesPlanted | ||
| key="speciesPlanted" | ||
| totalTreesCount={totalTreesCount} | ||
| plantedSpecies={plantLocationInfo.plantedSpecies} | ||
| /> | ||
| ), | ||
| plantLocationInfo && | ||
| plantLocationInfo.sampleInterventions && | ||
| plantLocationInfo.sampleInterventions.length > 0 && ( | ||
| <SampleTrees | ||
| key="sampleTrees" | ||
| sampleInterventions={plantLocationInfo.sampleInterventions} | ||
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | ||
| /> | ||
| ), | ||
| ].filter(Boolean); |
There was a problem hiding this comment.
Fix duplicate keys and improve conditional rendering.
The content array has several issues that need to be addressed:
- Duplicate key "plantingDetails"
- Complex conditional rendering that can be simplified
const content = [
<>
<InterventionHeader
plHid={plantLocationInfo?.hid}
interventionType={plantLocationInfo?.type}
plantDate={plantLocationInfo?.interventionStartDate}
key="interventionHeader"
/>
{shouldDisplayImageCarousel && (
<ImageSlider
key="imageSlider"
images={sampleInterventionSpeciesImages}
type="coordinate"
isMobile={isMobile}
imageSize="large"
allowFullView={!isMobile}
/>
)}
</>,
cleanedPublicMetadata.length > 0 && (
<OtherInterventionMetadata
- key="plantingDetails"
+ key="interventionMetadata"
metadata={cleanedPublicMetadata}
plantDate={plantLocationInfo?.interventionStartDate}
type={plantLocationInfo?.type}
/>),
plantLocationInfo?.plantedSpecies?.length > 0 && (
<SpeciesPlanted
key="speciesPlanted"
totalTreesCount={totalTreesCount}
plantedSpecies={plantLocationInfo.plantedSpecies}
/>
),
- plantLocationInfo &&
- plantLocationInfo.sampleInterventions &&
- plantLocationInfo.sampleInterventions.length > 0 && (
+ plantLocationInfo?.sampleInterventions?.length > 0 && (
<SampleTrees
key="sampleTrees"
sampleInterventions={plantLocationInfo.sampleInterventions}
setSelectedSamplePlantLocation={setSelectedSamplePlantLocation}
/>
),
].filter(Boolean);📝 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 content = [ | |
| <> | |
| <InterventionHeader | |
| plHid={plantLocationInfo?.hid} | |
| interventionType={plantLocationInfo?.type} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| key="interventionHeader" | |
| /> | |
| {shouldDisplayImageCarousel && ( | |
| <ImageSlider | |
| key="imageSlider" | |
| images={sampleInterventionSpeciesImages} | |
| type="coordinate" | |
| isMobile={isMobile} | |
| imageSize="large" | |
| allowFullView={!isMobile} | |
| /> | |
| )} | |
| </>, | |
| cleanedPublicMetadata.length > 0 && ( | |
| <OtherInterventionMetadata | |
| key="plantingDetails" | |
| metadata={cleanedPublicMetadata} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| type={plantLocationInfo?.type} | |
| />), | |
| plantLocationInfo?.plantedSpecies && | |
| plantLocationInfo.plantedSpecies.length > 0 && ( | |
| <SpeciesPlanted | |
| key="speciesPlanted" | |
| totalTreesCount={totalTreesCount} | |
| plantedSpecies={plantLocationInfo.plantedSpecies} | |
| /> | |
| ), | |
| plantLocationInfo && | |
| plantLocationInfo.sampleInterventions && | |
| plantLocationInfo.sampleInterventions.length > 0 && ( | |
| <SampleTrees | |
| key="sampleTrees" | |
| sampleInterventions={plantLocationInfo.sampleInterventions} | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| /> | |
| ), | |
| ].filter(Boolean); | |
| const content = [ | |
| <> | |
| <InterventionHeader | |
| plHid={plantLocationInfo?.hid} | |
| interventionType={plantLocationInfo?.type} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| key="interventionHeader" | |
| /> | |
| {shouldDisplayImageCarousel && ( | |
| <ImageSlider | |
| key="imageSlider" | |
| images={sampleInterventionSpeciesImages} | |
| type="coordinate" | |
| isMobile={isMobile} | |
| imageSize="large" | |
| allowFullView={!isMobile} | |
| /> | |
| )} | |
| </>, | |
| cleanedPublicMetadata.length > 0 && ( | |
| <OtherInterventionMetadata | |
| key="interventionMetadata" | |
| metadata={cleanedPublicMetadata} | |
| plantDate={plantLocationInfo?.interventionStartDate} | |
| type={plantLocationInfo?.type} | |
| />), | |
| plantLocationInfo?.plantedSpecies?.length > 0 && ( | |
| <SpeciesPlanted | |
| key="speciesPlanted" | |
| totalTreesCount={totalTreesCount} | |
| plantedSpecies={plantLocationInfo.plantedSpecies} | |
| /> | |
| ), | |
| plantLocationInfo?.sampleInterventions?.length > 0 && ( | |
| <SampleTrees | |
| key="sampleTrees" | |
| sampleInterventions={plantLocationInfo.sampleInterventions} | |
| setSelectedSamplePlantLocation={setSelectedSamplePlantLocation} | |
| /> | |
| ), | |
| ].filter(Boolean); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 185-186: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (1)
65-82: Remove unnecessary fragment and inline styles.The code can be simplified by removing the unnecessary fragment and moving inline styles to CSS module.
- <> {interventionData && ( <div className={styles.labelTextContainer}> {isMobile ? ( <label className={styles.interventionsLabel}> {truncateString(tIntervention(interventionData?.value), 40)} </label> ) : ( <p className={styles.interventionName} - style={{ marginTop: '5px' }} + className={`${styles.interventionName} ${styles.interventionNameSpacing}`} > {truncateString(tIntervention(interventionData?.value), 40)} </p> )} </div> )} - </>Add to CSS module:
.interventionNameSpacing { margin-top: 5px; }Also applies to: 74-76
🧰 Tools
🪛 Biome (1.9.4)
[error] 65-82: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
src/utils/constants/intervention.ts (2)
4-20: Encapsulate color constants.The color constants should be encapsulated in an object for better maintainability.
-const SINGLE_TREE = '#007A49'; -const MULTI_TREE = '#007A49'; -const INVASIVE_SPECIES = '#EB5757'; -// ... other color constants +const interventionColors = { + SINGLE_TREE: '#007A49', + MULTI_TREE: '#007A49', + INVASIVE_SPECIES: '#EB5757', + FIRE_SUPPRESSION: '#F2C94C', + FIRE_PATROL: '#F2994A', + // ... other colors +} as const;
75-79: Remove unnecessary index property.The
indexproperty inAllInterventionsseems unnecessary as it's always sequential and can be derived from array index if needed.export const AllInterventions: Array<{ label: string; value: INTERVENTION_TYPE; - index: number; }> = [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/features/projectsV2/ProjectDetails/components/microComponents/InterventionHeader.tsx(1 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx(1 hunks)src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx(1 hunks)src/utils/constants/intervention.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx
[error] 65-82: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
src/features/projectsV2/ProjectsMap/InterventionDropDown/InterventionList.tsx (1)
31-48: 🛠️ Refactor suggestionImprove accessibility and list rendering.
The list implementation needs several improvements:
- Using index as key in map function is an anti-pattern
- Missing accessibility attributes for interactive elements
- Class name
siteListOptionsshould be renamed to match intervention context- <ul className={styles.siteListOptions}> + <ul className={styles.interventionListOptions} role="listbox"> {interventionList.map((intervention, index) => { return ( <li className={`${styles.listItem} ${intervention.value === selectedInterventionData?.value ? styles.selectedItem : ''}`} onClick={() => handleFilterSelection(intervention.value)} - key={index} + key={intervention.value} + role="option" + aria-selected={intervention.value === selectedInterventionData?.value} + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleFilterSelection(intervention.value); + } + }} > <p>{tProjectDetails(intervention.value)}</p> </li> ); })} </ul>Likely invalid or redundant comment.
src/features/projectsV2/ProjectsMap/InterventionDropDown/index.tsx (2)
48-52:⚠️ Potential issueFix the useEffect logic condition.
The menu should close when
disableInterventionMenuis true, not false.useEffect(() => { - if (!disableInterventionMenu) { + if (disableInterventionMenu) { setIsMenuOpen(false); } }, [disableInterventionMenu]);Likely invalid or redundant comment.
39-46: 🛠️ Refactor suggestionAdd error boundary for undefined allInterventions.
The
useMemohook handles undefinedallInterventions, but it would be better to validate the prop at component level.+ if (!allInterventions) { + console.warn('InterventionDropdown: allInterventions prop is required'); + return null; + } const interventionList = useMemo(() => { - if (!allInterventions) return []; return allInterventions.map((el) => ({ label: el.label, index: el.index, value: el.value })); }, [allInterventions]);Likely invalid or redundant comment.
src/utils/constants/intervention.ts (1)
135-137: 🛠️ Refactor suggestionImprove type safety.
The function should use
INTERVENTION_TYPEinstead ofstringfor better type safety.-export const findMatchingIntervention = (value: string) => { +export const findMatchingIntervention = (value: INTERVENTION_TYPE) => { return AllInterventions.find((item) => item.value === value); };Likely invalid or redundant comment.
| @@ -0,0 +1,36 @@ | |||
| import styles from '../../styles/PlantLocationInfo.module.scss'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update class names and styles.
The component is reusing styles from PlantLocationInfo.module.scss and class names related to plant location. Create a new module for intervention-specific styles.
-import styles from '../../styles/PlantLocationInfo.module.scss';
+import styles from './InterventionHeader.module.scss';
- className={`plant-location-header-container ${styles.plantLocationHeaderContainer}`}
+ className={styles.interventionHeaderContainer}Also applies to: 20-21
mariahosfeld
dismissed
sunilsabatp’s stale review
All remarks should be resolved.
| const shouldShowOtherIntervention = | ||
| props.isMobile && | ||
| selectedPlantLocation !== null && | ||
| !PLANTATION_TYPES.includes(selectedPlantLocation.type); |
There was a problem hiding this comment.
This code can be optimized by using the existing helper function isNonPlantationType as following:
const shouldShowOtherIntervention = props.isMobile && isNonPlantationType(selectedPlantLocation);
5 participants
Fixes #
Changes in this pull request:
Displaying other interventions in the web app.