Enhance Precise Location Manipulation on Chapters Map #2914
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new boolean prop Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/ChapterMap.tsx (1)
121-147: Ensure user-marker removal is always safe; consider unmount cleanup
CallinguserMarkerRef.current.remove()is fine for de-duping, but if this component can ever unmount/remount without Leaflet map teardown, you may leak handlers/layers. Consider adding auseEffect(() => () => mapRef.current?.remove(), [])teardown (and nulling refs) if remounts are possible in your app’s navigation patterns.frontend/src/components/ChapterMapWrapper.tsx (1)
23-29: Drop the eslint-disable by using the destructured value (or underscore it)
Current code disables@typescript-eslint/no-unused-varsjust to omit the prop. Prefer either (a) deriveenableLocationSharingfrom the destructured value, or (b) rename it to_showLocationSharingwhen destructuring.-import React, { useState } from 'react' +import React, { useState } from 'react' import type { Chapter } from 'types/chapter' import { getUserLocationFromBrowser, sortChaptersByDistance, type UserLocation, } from 'utils/geolocationUtils' const ChapterMap = dynamic(() => import('./ChapterMap'), { ssr: false }) const ChapterMapWrapper: React.FC<ChapterMapWrapperProps> = (props) => { const [userLocation, setUserLocation] = useState<UserLocation | null>(null) const [sortedData, setSortedData] = useState<Chapter[] | null>(null) - const enableLocationSharing = props.showLocationSharing === true - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { showLocationSharing, ...mapProps } = props + const { showLocationSharing, ...mapProps } = props + const enableLocationSharing = showLocationSharing === true if (!enableLocationSharing) { return <ChapterMap {...mapProps} /> }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/src/app/community/snapshots/[id]/page.tsx(1 hunks)frontend/src/app/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(1 hunks)frontend/src/components/ChapterMap.tsx(4 hunks)frontend/src/components/ChapterMapWrapper.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
🔇 Additional comments (7)
frontend/src/components/ChapterMap.tsx (2)
31-32: Good: persist user marker instance viauserMarkerRef
This is the right direction for enforcing “at most one” precise-location pin across renders.
56-69: Fix matches objective: don’t deactivate map when moving to controls near the map
ThemapParent.contains(relatedTarget)guard should prevent the “Click to interact with map” overlay from reappearing when hovering/clicking UI that lives in the map wrapper (e.g., location-sharing button).frontend/src/app/community/snapshots/[id]/page.tsx (1)
138-143: LGTM: enables consistent location sharing on snapshot “New Chapters” mapfrontend/src/components/CardDetailsPage.tsx (1)
174-189: LGTM: chapter details map now opts into location sharingfrontend/src/components/ChapterMapWrapper.tsx (1)
55-62: Verify wrapper sizing change doesn’t break maps in non-fixed-height parents
Switching from"space-y-4"to"h-full w-full"may rely on parent containers providing an explicit height; otherwise Leaflet can render a 0px-tall map.frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
104-125: LGTM: test mock updated for newshowLocationSharingpropfrontend/src/app/page.tsx (1)
311-322: LGTM: enables location sharing on home “Chapters Worldwide” map
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 Thanks for working on these updates so quickly!
A couple of things I noticed:
- Precise location button works differently on chapters page and on the home page. On chapters page resetting shared location moves back to a general view. On the home page, however, it stays in that zoomed in local view (as if I'm still on the precise location view, but without the pin).
2 more I shared in comment below 🔽
| <ChapterMapWrapper | ||
| geoLocData={geolocationData} | ||
| showLocal={true} | ||
| showLocationSharing={true} |
There was a problem hiding this comment.
This didn't work for me on an individual chapter's page. It's still showing my hardcoded IP location instead of the precise geolocation.
There was a problem hiding this comment.
Hy @kasya for individual chapter it actually focusing on the individual chapter location but currently fixed it so when user share location it will properly shows both the user location pin and the chapter pins
| <ChapterMapWrapper | ||
| geoLocData={snapshot.newChapters} | ||
| showLocal={false} | ||
| showLocationSharing={true} |
There was a problem hiding this comment.
This also doesn't work for me right now. It just refreshes the chapter without any movement to a precise location
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
35-190: Add cleanup function to prevent potential memory leaks.The
useEffectcreates Leaflet objects (map, marker cluster, user marker) and attaches event listeners, but doesn't clean them up on unmount. Consider adding a cleanup function:useEffect(() => { // ... existing code ... return () => { if (userMarkerRef.current) { userMarkerRef.current.remove() userMarkerRef.current = null } if (markerClusterRef.current) { markerClusterRef.current.clearLayers() markerClusterRef.current = null } if (mapRef.current) { mapRef.current.off() mapRef.current.remove() mapRef.current = null } } }, [geoLocData, showLocal, userLocation])Note: Place the return statement at the end of the
useEffect, after all the existing logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(6 hunks)
🔇 Additional comments (7)
frontend/src/components/ChapterMap.tsx (7)
31-32: LGTM! Refs enable proper marker lifecycle management.The
userMarkerRefensures only one user location pin exists at a time, andinitialViewRefenables restoring the default map view when location sharing is disabled—both directly address the PR objectives.
47-50: LGTM! Initial view correctly captured.Capturing the initial center and zoom immediately after map creation ensures the default view can be restored when location sharing is disabled.
66-71: LGTM! Mouseout logic correctly handles button interactions.Checking whether the
relatedTargetis within the map's parent element prevents the "Click to interact with map" overlay from reappearing when hovering over the share location button, directly addressing one of the PR objectives.
127-130: LGTM! Prevents duplicate user location pins.Removing any existing user marker before creating a new one guarantees at most one pin exists, directly addressing the PR objective about preventing multiple pins when activating shared location mode repeatedly.
152-152: LGTM! Completes marker lifecycle tracking.Storing the newly created marker in the ref enables its removal on subsequent renders, completing the single-pin guarantee mechanism.
158-164: LGTM! Bounds calculation correctly includes user location.Constructing
locationsForBoundswith the user location followed by nearby chapters ensures the map view encompasses both, improving the user experience when location sharing is enabled. The filter at lines 89-93 ensures all coordinates are numbers at runtime.
187-188: LGTM! Properly restores initial view.When location sharing is disabled and no local chapters are displayed, restoring the initial view ensures a consistent default state, directly addressing the PR objective about fully resetting the map when disabling shared location mode.
|
The issue has been fixed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/ChapterMap.tsx (2)
158-168: LGTM! User location included in bounds calculation.Including the user's location as the first point in the bounds array ensures the map fits both the user's position and nearby chapters, providing good spatial context.
Note: This branch considers 5 chapters while the
showLocal-only branch (line 173) considers 4. This appears intentional but might warrant a code comment explaining the difference.
35-193: Consider extracting bounds calculation logic for improved readability.The useEffect contains complex view management logic with multiple conditional branches. While the current implementation is correct, extracting the bounds calculation logic into separate helper functions could improve maintainability.
Example structure:
const calculateBoundsWithUserLocation = (userLocation, chapters, maxCount) => { ... } const calculateLocalBounds = (chapters, maxCount) => { ... }This would make the main effect logic more declarative and easier to test in isolation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(2 hunks)frontend/src/components/ChapterMap.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (7)
frontend/__tests__/unit/components/ChapterMap.test.tsx (2)
11-13: LGTM! Mock additions support new map view management.The mock implementations for
getCenter,getZoom, andgetContainercorrectly support the new functionality in ChapterMap that captures the initial view and handles mouseout containment checks.
357-361: LGTM! More resilient test assertion strategy.Capturing the initial call count and asserting a relative increase (rather than an exact total) makes the test more robust as the view management logic evolves with new features like user location markers and initial view restoration.
frontend/src/components/ChapterMap.tsx (5)
31-32: LGTM! Refs enable single-marker pattern and view restoration.The
userMarkerRefensures only one user location marker exists at a time (preventing duplicates per PR objectives), whileinitialViewRefenables restoring the original map view when disabling location sharing.
47-50: LGTM! Initial view captured at the right time.Capturing the map's center and zoom immediately after initialization ensures the fallback restoration (lines 190-191) resets to the correct default view.
66-71: LGTM! Mouseout logic prevents overlay reappearance on button hover.By checking whether the
relatedTargetis contained within the map's parent element, the mouseout handler correctly prevents the "Click to interact with map" overlay from reappearing when hovering over the location sharing button. This directly addresses the PR objective to prevent button hover from causing loss of map focus.
127-130: LGTM! User marker cleanup prevents duplicates.Removing the existing user marker before potentially creating a new one ensures that only one precise location pin exists on the map at any time, directly addressing PR objective to prevent multiple pins.
190-191: LGTM! Fallback ensures clean reset behavior.Restoring the initial view when neither user location nor local view conditions are met provides clean reset behavior when disabling location sharing, directly supporting the PR objective to fully reset the map state.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 Works good! Thanks!
2 participants
Resolves #2895
Proposed change
This PR fixes the following issues with precise location on Chapter maps
Checklist
make check-testlocally and all tests passed