Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork103
Map Glitches Due to Unnecessary Re-Renders#1156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedMar 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 withCodeRabbit:
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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>${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>${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.
Uh oh!
There was an error while loading.Please reload this page.
| `<div class="popup-content">${chapter.name}</div>` | ||
| ) | ||
| marker.on('click', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
map.fitBounds() does not ensure that the nearest chapter gets focused first before fitting bounds, this will lead to unexpected zoom behavior, because thesetView() 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
handle the condition where themarkerClusterRef.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please remove all the comments from the file they are not necessary here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I mentioned earlier, please check outNest-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| letclicked=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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you can use the previous logic it will work fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks@abhayymishraa for your suggestions, i have made the changes according to what you have said, can you review that once. thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work@srinjoy933 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks as it was before now 👍🏼 Thanks for updating that!
Left a request forpre-commit-config file.
However, I don't see any use ofuseCallback 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@srinjoy933 why you keep removing this hook? 🤔 Please revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@kasya will it be okay for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Works great! 👍🏼
0af1ad6Uh oh!
There was an error while loading.Please reload this page.
* 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>
Uh oh!
There was an error while loading.Please reload this page.
Resolves#1128
Optimized marker management using clearLayers() instead of re-adding markers.
Ensured single map initialization with useEffect.