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.
Dishant1804
left a comment
Dishant1804
left a comment
There was a problem hiding this comment.
Overall the useMemo and useCallback hook will improve the performance and avoid unnecessary re renders. please make the suggested changes
Thank you :)
| 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.
| })) | ||
| }, [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 :)
Rajgupta36
left a comment
Rajgupta36
left a comment
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
Rajgupta36
left a comment
Rajgupta36
left a comment
There was a problem hiding this comment.
Nice work @srinjoy933 👍
| - --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.
@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? |
|
* changes for the issue-1128 * updated * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update ChapterMap.tsx * Update .pre-commit-config.yaml * Update ChapterMap.tsx * Update .pre-commit-config.yaml * your commit message --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Kate Golovanova <kate@kgthreads.com>
6 participants
Resolves #1128
Optimized marker management using clearLayers() instead of re-adding markers.
Ensured single map initialization with useEffect.