Map Glitches Due to Unnecessary Re-Renders #1156
Conversation
Summary by CodeRabbit
WalkthroughThe pull request refactors the marker cluster logic in the ChapterMap component. The code now conditionally initializes the marker cluster group and, if already set, clears its layers instead of removing and re-adding markers individually. Additionally, marker creation is refactored to generate an array using the Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
Documentation and Community
|
![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
🧹 Nitpick comments (3)
frontend/src/components/ChapterMap.tsx (3)
34-47: Consider extracting map configuration to constants.While the current implementation is correct, the map configuration could be extracted to named constants for better readability and maintainability.
+const DEFAULT_MAP_VIEW = [20, 0]; +const DEFAULT_ZOOM_LEVEL = 2; +const MAP_BOUNDS = [[-90, -180], [90, 180]]; +const TILE_LAYER_URL = 'https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png'; +const TILE_LAYER_ATTRIBUTION = '© OpenStreetMap contributors'; useEffect(() => { if (!mapRef.current) { const map = L.map('chapter-map', { worldCopyJump: false, maxBounds: MAP_BOUNDS, maxBoundsViscosity: 1.0, - }).setView([20, 0], 2) + }).setView(DEFAULT_MAP_VIEW, DEFAULT_ZOOM_LEVEL) - L.tileLayer('https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png', { - attribution: '© OpenStreetMap contributors', + L.tileLayer(TILE_LAYER_URL, { + attribution: TILE_LAYER_ATTRIBUTION, className: 'map-tiles', }).addTo(map)
67-88: Consider extracting marker creation into a separate function.The marker creation logic could be extracted into a separate function for better code organization and testability.
+const createMarker = (chapter) => { + const markerIcon = new L.Icon({ + iconAnchor: [12, 41], + iconRetinaUrl: '/img/marker-icon-2x.png', + iconSize: [25, 41], + iconUrl: '/img/marker-icon.png', + popupAnchor: [1, -34], + shadowSize: [41, 41], + shadowUrl: '/img/marker-shadow.png', + }); + + const marker = L.marker([chapter.lat, chapter.lng], { icon: markerIcon }).bindPopup( + `<div class="popup-content">${chapter.name}</div>` + ); + + marker.on('click', () => { + window.location.href = `/chapters/${chapter.key}`; + }); + + return marker; +}; // In the updateMarkers function: chapters.forEach((chapter) => { - const markerIcon = new L.Icon({ - iconAnchor: [12, 41], - iconRetinaUrl: '/img/marker-icon-2x.png', - iconSize: [25, 41], - iconUrl: '/img/marker-icon.png', - popupAnchor: [1, -34], - shadowSize: [41, 41], - shadowUrl: '/img/marker-shadow.png', - }); - - const marker = L.marker([chapter.lat, chapter.lng], { icon: markerIcon }).bindPopup( - `<div class="popup-content">${chapter.name}</div>` - ); - - marker.on('click', () => { - window.location.href = `/chapters/${chapter.key}`; - }); + const marker = createMarker(chapter); markerClusterGroup.addLayer(marker); bounds.push([chapter.lat, chapter.lng]); });
68-77: Consider using useMemo for the marker icon.The marker icon configuration is recreated for each marker but remains constant. Consider using
useMemoto create it once.+// Create marker icon once +const markerIcon = useMemo(() => new L.Icon({ + iconAnchor: [12, 41], + iconRetinaUrl: '/img/marker-icon-2x.png', + iconSize: [25, 41], + iconUrl: '/img/marker-icon.png', + popupAnchor: [1, -34], + shadowSize: [41, 41], + shadowUrl: '/img/marker-shadow.png', +}), []); // In the updateMarkers function: chapters.forEach((chapter) => { - const markerIcon = new L.Icon({ - iconAnchor: [12, 41], - iconRetinaUrl: '/img/marker-icon-2x.png', - iconSize: [25, 41], - iconUrl: '/img/marker-icon.png', - popupAnchor: [1, -34], - shadowSize: [41, 41], - shadowUrl: '/img/marker-shadow.png', - }); const marker = L.marker([chapter.lat, chapter.lng], { icon: markerIcon })...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(5 hunks)
🔇 Additional comments (9)
frontend/src/components/ChapterMap.tsx (9)
2-2: Properly added React hooks imports.The addition of
useMemoanduseCallbackimports is correct and necessary for the performance optimizations made in this component.
21-21: Clear comment indicating purpose of useMemo.Good practice to add explanatory comments for React hooks usage.
31-50: Improved map initialization pattern.The map initialization has been properly isolated in its own effect with an empty dependency array to ensure it only runs once. Creating a local
mapvariable before assigning to the ref improves code clarity.
52-53: Informative comment for the new updateMarkers function.The comment clearly explains the purpose of this function which helps with maintainability.
53-101: Effective use of useCallback for optimization.Good implementation of
useCallbackto memoize theupdateMarkersfunction with the correct dependencies[chapters, showLocal]. This prevents unnecessary function recreations during re-renders, directly addressing the PR objective of reducing map glitches.
57-63: Optimized marker management.The new approach of clearing existing layers instead of recreating the marker cluster group is more efficient. The conditional logic properly handles both the initial case and subsequent updates.
78-80: Simplified popup binding.The refactored popup creation using
bindPopupdirectly with a template string makes the code more concise and readable.
95-95: Streamlined local chapters selection.The modified slice operation more directly expresses the intent to include the maximum number of nearest chapters.
103-106: Proper effect for marker updates.The new effect correctly triggers the
updateMarkersfunction when its dependencies change. This separation of concerns improves code organization and ensures markers are updated when needed.
| `<div class="popup-content">${chapter.name}</div>` | ||
| ) | ||
|
|
||
| marker.on('click', () => { |
There was a problem hiding this comment.
Please don't change the map behavior on the first click to the marker.
On the first click, it should display a popup with the chapter name, and only on the second click to the name should it redirect to that specific chapter.
| const nearestChapter = chapters[0] | ||
| map.setView([nearestChapter.lat, nearestChapter.lng], maxZoom) | ||
| map.fitBounds(localBounds, { maxZoom: maxZoom }) | ||
|
|
There was a problem hiding this comment.
map.fitBounds() does not ensure that the nearest chapter gets focused first before fitting bounds, this will lead to unexpected zoom behavior, because the setView() is removed which was responsible for setting the initial zoomed view on nearest chapter.
Please fix it @srinjoy933
| // Create a new marker cluster group | ||
| const markerClusterGroup = L.markerClusterGroup() | ||
| // Clear existing markers if needed | ||
| if (markerClusterRef.current) { |
There was a problem hiding this comment.
handle the condition where the markerClusterRef.current is null, because if the previous markers are not cleared properly this may lead to duplicate markers while re rendering.
| @@ -27,9 +28,10 @@ const ChapterMap = ({ | |||
| })) | |||
| }, [geoLocData]) | |||
|
|
|||
| // Function to initialize map (runs once) | |||
There was a problem hiding this comment.
please remove all the comments from the file they are not necessary here :)
There was a problem hiding this comment.
Agree with @abhayymishraa and @Dishant1804 comments. Try not to store tiles in the cache, as it can severely impact memory usage. This is a world map, not just for a specific place.
|
Agree with @Rajgupta36 and @Dishant1804 Storing cache will lead here to memory leak because Leaflet fetches images according to the zoom |
|
thanks @abhayymishraa , @Dishant1804 , @Rajgupta36 for the suggestions . i will make the changes and come with the updated one |
|
@abhayymishraa , @Dishant1804 , @Rajgupta36 can you please review this and tell me ,is there any changes required .thank you. |
| let clicked = false | ||
| popupContent.addEventListener('click', () => { | ||
| if (clicked) { | ||
| window.location.href = `/chapters/${chapter.key}` | ||
| } | ||
| clicked = true | ||
| setTimeout(() => (clicked = false), 500) |
There was a problem hiding this comment.
As I mentioned earlier, please check out Nest-Chapters.
When you click on a marker a tooltip appears. Clicking on the name inside the tooltip then it redirects you to that chapter.
We need this same behaviour as this task is for optimizing the re-renders.
| let clicked = false | ||
| popupContent.addEventListener('click', () => { | ||
| if (clicked) { | ||
| window.location.href = `/chapters/${chapter.key}` | ||
| } | ||
| clicked = true | ||
| setTimeout(() => (clicked = false), 500) | ||
| }) |
There was a problem hiding this comment.
| let clicked = false | |
| popupContent.addEventListener('click', () => { | |
| if (clicked) { | |
| window.location.href = `/chapters/${chapter.key}` | |
| } | |
| clicked = true | |
| setTimeout(() => (clicked = false), 500) | |
| }) | |
| popupContent.addEventListener('click', () => { | |
| window.location.href = `/chapters/${chapter.key}` | |
| }) |
There was a problem hiding this comment.
you can use the previous logic it will work fine
There was a problem hiding this comment.
thanks @abhayymishraa for your suggestions, i have made the changes according to what you have said, can you review that once. thank you
…/Nest into srinjoy933/issue-1128
|
the recent one
|
|
any more changes regarding this @arkid15r ? |
There was a problem hiding this comment.
When we get to the page for the first time the map should show local zoom and not the whole world map.
Example, existing view:
View in this PR:
There was a problem hiding this comment.
Why did we change the style/look of the map? I'd prefer to keep it as it was before, with map detailed map with colors for different types of landscape, roads, etc.
| icon: markerIcon | ||
| }); | ||
|
|
||
| // Add popup |
There was a problem hiding this comment.
The name content in the popup should be clickable and lead to that chapter's page.
Example, current view:
View in your PR:
Please reference https://nest.owasp.dev/chapters for all the style issues. We want to keep the map as close to how it looked before this changes, since it was not style related issue.
There was a problem hiding this comment.
The map stays light colored in dark mode. We need to have a dark mode map, and ideally it should match what we currently have in https://nest.owasp.dev/chapters
Also, in dark mode I can see that the tooltip point has dark color, where the rest is white. This whole thing should have dark background:
There was a problem hiding this comment.
One more thing I noticed.
When there's grouping for chapters - there's no circle for it anymore. For example, here on the map there's a number 4 and no mark for it at all. You can't really see it - there should be a circle to highlight this.
Again, please reference how it looks on https://nest.owasp.dev/chapters
| import React, { useEffect, useMemo, useRef, useCallback } from 'react'; | ||
| import 'leaflet/dist/leaflet.css'; | ||
| import 'leaflet.markercluster/dist/MarkerCluster.css'; | ||
| import 'leaflet.markercluster/dist/MarkerCluster.Default.css'; |
There was a problem hiding this comment.
Also, please run checks before submitting code with make check-test-all. There are many unnecessary changes, like added ; for example. This makes it hard to review, since we have to dig through irrelevant code.
We do not use ; at the end of the lines. Running make check-test-all will clean this up for you.
| return ( | ||
| <div> | ||
| <style>{` | ||
| .custom-popup-container .leaflet-popup-content-wrapper { |
There was a problem hiding this comment.
I guess this is the reason for a completely new look.
Could you remove/update this?
There was a problem hiding this comment.
yes for sure .
i have updated it as per the changes you suggested can you check it now and suggest any further changes is required or not.thank you @kasya
There was a problem hiding this comment.
This looks as it was before now 👍🏼 Thanks for updating that!
Left a request for pre-commit-config file.
However, I don't see any use of useCallback now 🤔 . If it's not necessary anymore, could you update the description of the PR? Just asking because it was mentioned in the description of this issue/PR.
I do like that we now add and remove markers in single bulk operations 👍🏼
Once it's cleaned up and updated - we can merge this in!
.pre-commit-config.yaml
Outdated
| @@ -26,17 +26,6 @@ repos: | |||
| types: | |||
| - html | |||
|
|
|||
| - repo: https://github.com/jumanjihouse/pre-commit-hook-yamlfmt | |||
There was a problem hiding this comment.
@srinjoy933 why you keep removing this hook? 🤔 Please revert this change.
There was a problem hiding this comment.
yes i made this change now .hope it is all ok and ready to merge.
.pre-commit-config.yaml
Outdated
| @@ -35,7 +35,7 @@ repos: | |||
| - --mapping=2 | |||
| - --offset=2 | |||
| - --sequence=4 | |||
| exclude: (.github|frontend/pnpm-lock.yaml) | |||
| exclude: (.github|frontend/pnpm-lock.yaml|^\.pre-commit-config\.yaml$) | |||
There was a problem hiding this comment.
@srinjoy933 why this change? 🤔 Can we keep it as before? The rest looks good and ready to go!
There was a problem hiding this comment.
@ kasya can we make it same.as it will not hamper any other such hooks.but due to that hook only many a times test fails ,due to that reason i made it exclude that particular file only ,
There was a problem hiding this comment.
i myself faced problem in the code quality tests, so i excluded it for that file only. will it be okay to make it there, as it dont hamper in any other cases
There was a problem hiding this comment.
@kasya will it be okay for you?
There was a problem hiding this comment.
@srinjoy933 it doesn't seem to be the problem for other contributors though. 🤔
If you're not sure how to make it not fail the tests you can always ask in our Slack channel for guidance from other contributors.
|
|
i solved the issue and now i think it is ready to merge. @kasya is it fine now? |
Resolves #1128
Optimized marker management using clearLayers() instead of re-adding markers.
Ensured single map initialization with useEffect.