Tech - Correction de la fuite mémoire avec le state `vessels` #3742

louptheron · 2024-10-04T16:41:42Z

Linked issues

Resolve Intégrer les changements de l'audit de perf #886

Filtres

Création de features/Filter
Sauvegarde des filtres avec persistReducerTyped et non plus en direct avec le localstorage

Navires

Création de features/Vessel
Ajout d'un createEntityAdapter pour les dernières positions des navires (vessels)
Normalisation des navires vessels : suppression du sous-objet vesselProperties
Ajout de use-cases redux-thunk pour intégagir avec la layer VesselsLayer d'OL
Utilisation de vesselsAdapter.setMany pour éviter les fuites mémoires
Sortie de VESSELS_VECTOR_SOURCE et VESSELS_VECTOR_LAYER dans une constante, en dehors d'un composant React
Utilise featuer.get au lieu de feature.getProperties pour éviter un shallow copy des propriétés

Liste des navires

Ajout d'une condition sur VesselList pour ne pas écouter vessels dès le démarrage de l'app
Migration des useState locaux dans le slice

Tests E2E (Cypress)

ivangabriele

Nicely done :) Y a plein de comments mais presque tous sont des questions de refacto / goûts.

J'ai vu plein d'import COLORS qui viennent encore de constants, j'ai arrêté les comments liés à ça du coup. Si tu as la motiv tu peux les retirer, sinon c'est pas grave ça sera fait un jour !

ivangabriele · 2024-10-08T15:42:19Z

frontend/src/features/VesselSearch/constants.ts

-    ['vesselProperties', 'mmsi'],
-    ['vesselProperties', 'ircs']
-  ],
+  keys: [['vesselName'], ['internalReferenceNumber'], ['externalReferenceNumber'], ['mmsi'], ['ircs']],


On garde les arrays dans l'array ?

ivangabriele · 2024-10-08T15:49:12Z

frontend/src/features/Vessel/useCases/renderVessels.ts

+
+import type { MainAppThunk } from '@store'
+
+export const renderVessels = (): MainAppThunk => async (_, getState) => {


Suggested change

export const renderVessels = (): MainAppThunk => async (_, getState) => {

export const renderVesselFeatures = (): MainAppThunk => (_, getState) => {

?

ivangabriele · 2024-10-08T15:50:28Z

frontend/src/features/Vessel/useCases/showAlertInSideWindow.ts

@@ -15,7 +15,7 @@ export const showAlertInSideWindow =
        flagState: selectedVessel.flagState,
        internalReferenceNumber: selectedVessel.internalReferenceNumber,
        ircs: selectedVessel.ircs,
-        name: selectedVessel?.alerts[0] ?? null
+        name: selectedVessel?.alerts?.length ? selectedVessel?.alerts[0] : null


Suggested change

name: selectedVessel?.alerts?.length ? selectedVessel?.alerts[0] : null

name: selectedVessel.alerts?.length ? selectedVessel?.alerts[0] : null

ivangabriele · 2024-10-08T15:54:36Z

frontend/src/features/Vessel/useCases/previewVessels.ts

+      // @ts-ignore
+      const extent = getExtentFromGeoJSON(zonesSelected[0]?.feature)
+      if (extent?.length && !isNumeric(extent[0]) && !isNumeric(extent[1])) {
+        dispatch(setProcessingRegulationSearchedZoneExtent(extent))


Je sais que ce n'est pas créé sur cette PR mais le nommer setSelectedRegulationZoneExtent() ne serait pas plus clair ?

ivangabriele · 2024-10-08T15:59:57Z

frontend/src/features/Vessel/useCases/previewVessels.ts

+      const extent = getExtentFromGeoJSON(zonesSelected[0]?.feature)
+      if (extent?.length && !isNumeric(extent[0]) && !isNumeric(extent[1])) {
+        dispatch(setProcessingRegulationSearchedZoneExtent(extent))
+        dispatch(animateToExtent())


Peut-être en TODO mineure pour plus tard mais je trouve ça plus clair quand une action UI est "auto-contenue". Du genre (en pseudo-code) setExtent(nextExtent: [...], withAnimation: true) plutôt que de setter d'abord une valeur pour ensuite déclencher l'animation via une autre action qui utilise cette valeur.

ivangabriele · 2024-10-08T17:06:12Z

frontend/src/features/Filter/components/VesselFilters/Filters.tsx

+  const filters = useMainAppSelector(state => state.filter.filters)
+  const rightMapBoxOpened = useMainAppSelector(state => state.global.rightMapBoxOpened)
+
+  const isOpen = useMemo(() => rightMapBoxOpened === MapBox.FILTERS, [rightMapBoxOpened])


Suggested change

const isOpen = useMemo(() => rightMapBoxOpened === MapBox.FILTERS, [rightMapBoxOpened])

const isOpen = rightMapBoxOpened === MapBox.FILTERS

?

ivangabriele · 2024-10-08T17:11:19Z

frontend/src/features/Vessel/layers/VesselAlertLayer.jsx

@@ -81,18 +71,18 @@ const VesselAlertLayer = () => {
      const { vesselIsHidden, vesselIsOpacityReduced } = getVesselLastPositionVisibilityDates(vesselsLastPositionVisibility)

      const features = vessels.reduce((features, vessel) => {
-        if (!vessel.vesselProperties.hasAlert) return features
+        if (!vessel.hasAlert) return features


Un switch(true) serait plus clean à mon goût mais c'est tout à fait personnel.

+ Même remarque pour les autres layers.

ivangabriele · 2024-10-08T17:16:03Z

frontend/src/features/Vessel/layers/VesselAlertLayer.jsx

+  const hideNonSelectedVessels = useSelector(state => state.vessel.hideNonSelectedVessels)
+  const vesselsTracksShowed = useSelector(state => state.vessel.vesselsTracksShowed)
+  const selectedVesselIdentity = useSelector(state => state.vessel.selectedVesselIdentity)
+  const vessels = useSelector(state => vesselSelectors.selectAll(state.vessel.vessels))
+  const nonFilteredVesselsAreHidden = useSelector(state => state.filter.nonFilteredVesselsAreHidden)
+  const previewFilteredVesselsMode = useSelector(state => state.global.previewFilteredVesselsMode)
+  const hideVesselsAtPort = useSelector(state => state.map.hideVesselsAtPort)
+  const vesselsLastPositionVisibility = useSelector(state => state.map.vesselsLastPositionVisibility)


Suggested change

const hideNonSelectedVessels = useSelector(state => state.vessel.hideNonSelectedVessels)

const vesselsTracksShowed = useSelector(state => state.vessel.vesselsTracksShowed)

const selectedVesselIdentity = useSelector(state => state.vessel.selectedVesselIdentity)

const vessels = useSelector(state => vesselSelectors.selectAll(state.vessel.vessels))

const nonFilteredVesselsAreHidden = useSelector(state => state.filter.nonFilteredVesselsAreHidden)

const previewFilteredVesselsMode = useSelector(state => state.global.previewFilteredVesselsMode)

const hideVesselsAtPort = useSelector(state => state.map.hideVesselsAtPort)

const vesselsLastPositionVisibility = useSelector(state => state.map.vesselsLastPositionVisibility)

const {

hideNonSelectedVessels,

vesselsTracksShowed,

// ...

} = useMainAppSelector(state => ({

hideNonSelectedVessels: state.vessel.hideNonSelectedVessels,

vesselsTracksShowed: state.vessel.vesselsTracksShowed,

// ...

}))

+ Même remarque pour les autres layers.

ivangabriele · 2024-10-08T17:21:33Z

frontend/src/features/Vessel/layers/VesselAlertLayer.jsx

+} from '../../../domain/entities/vessel/vessel'
+import { useIsSuperUser } from '../../../auth/hooks/useIsSuperUser'
+import { monitorfishMap } from '../../map/monitorfishMap'
+import { vesselsAdapter, vesselSelectors } from '../slice'

 const VesselAlertLayer = () => {


Proposition de TODO : j'en ferais plutôt un hook en memoizant simplement les features plutût qu'un composant memoizé sans rendu.

+ Même remarque pour les autres layers.

Autre PR please ^^

ivangabriele · 2024-10-08T17:26:17Z

frontend/src/features/Vessel/layers/VesselsLabelsLayer.tsx

@@ -228,18 +233,39 @@ export function VesselsLabelsLayer({ mapMovingAndZoomEvent }) {
      }

      const nextFeaturesAndLabels = features.map(feature => {
-        const { vesselProperties } = feature
+        const vesselProperties = {


Je ne sais pas si c'est utilisé à plus de deux endroits mais je me pose la question d'un petit util selon ^^.

sonarqubecloud · 2024-10-09T11:40:01Z

Quality Gate passed

Issues
95 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

louptheron marked this pull request as ready for review October 8, 2024 12:57

louptheron added 7 commits October 8, 2024 14:57

wip: move to vesselsAdapter

241dc31

Use redux rtk entity

68789ba

Refactor Filter slice

02f3a70

Add use-case from preview vessel in vessels list

8540a0e

Fix lint

b826275

Fix selector

de8cb34

Use precise selectors

93de412

louptheron force-pushed the loup/fix_memory_leak_on_vessels branch from aebd5a4 to 93de412 Compare October 8, 2024 12:57

louptheron requested a review from ivangabriele October 8, 2024 12:58

Fix vessel list filter by zone

1db85ff

ivangabriele reviewed Oct 8, 2024

View reviewed changes

louptheron added 5 commits October 9, 2024 09:15

Move vessel list filters to slice

3e99d74

Apply various review comments

5aab5cc

Force as non-null

95f1782

Fix multi cascader

88401bf

Increment wait in cypress

882f1d8

louptheron merged commit 79ef835 into master Oct 9, 2024
26 checks passed

louptheron deleted the loup/fix_memory_leak_on_vessels branch October 9, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tech - Correction de la fuite mémoire avec le state `vessels` #3742

Tech - Correction de la fuite mémoire avec le state `vessels` #3742

louptheron commented Oct 4, 2024 •

edited

Loading

ivangabriele left a comment

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

ivangabriele Oct 8, 2024

louptheron Oct 9, 2024

ivangabriele Oct 8, 2024

sonarqubecloud bot commented Oct 9, 2024


		import type { MainAppThunk } from '@store'

		export const renderVessels = (): MainAppThunk => async (_, getState) => {

	export const renderVessels = (): MainAppThunk => async (_, getState) => {
	export const renderVesselFeatures = (): MainAppThunk => (_, getState) => {

	name: selectedVessel?.alerts?.length ? selectedVessel?.alerts[0] : null
	name: selectedVessel.alerts?.length ? selectedVessel?.alerts[0] : null

	const isOpen = useMemo(() => rightMapBoxOpened === MapBox.FILTERS, [rightMapBoxOpened])
	const isOpen = rightMapBoxOpened === MapBox.FILTERS

Tech - Correction de la fuite mémoire avec le state vessels #3742

Tech - Correction de la fuite mémoire avec le state vessels #3742

Conversation

louptheron commented Oct 4, 2024 • edited Loading

Linked issues

Filtres

Navires

Liste des navires

ivangabriele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 9, 2024

Quality Gate passed

Tech - Correction de la fuite mémoire avec le state `vessels` #3742

Tech - Correction de la fuite mémoire avec le state `vessels` #3742

louptheron commented Oct 4, 2024 •

edited

Loading