- Notifications
You must be signed in to change notification settings - Fork95
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
Map Glitches Due to Unnecessary Re-Renders#1156
base:main
Are you sure you want to change the base?
Conversation
Summary by CodeRabbit
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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.
| `<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 👍
the recent one
|
any more changes regarding this@arkid15r ? |
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 for working on this! Left some comments ⬇️
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The name content in the popup should be clickable and lead to that chapter's page.
Example, current view:
View in your PR:
Please referencehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 inhttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 onhttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, please run checks before submitting code withmake 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. Runningmake 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess this is the reason for a completely new look.
Could you remove/update this?
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 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.
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.
| @@ -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?
Resolves#1128
Optimized marker management using clearLayers() instead of re-adding markers.
Ensured single map initialization with useEffect.