Add precise location sharing option for chapter map#2644
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds opt-in browser geolocation: new geolocation utilities, ChapterMap and ChapterMapWrapper accept userLocation and onShareLocation, chapters can be sorted by proximity and map recenters when enabled, and the Chapters page enables the location-sharing UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 (5)
frontend/src/utils/geolocationUtils.ts (3)
31-59: EnsuregetUserLocationFromBrowserstays client‑only and consider explicit “denied” handlingThe geolocation flow and error handling are reasonable, but this helper assumes a browser environment (
navigator.geolocation). Make sure it’s only imported and called from client components, and consider whether you want to distinguish “denied / failed” from “not requested yet” in the UI instead of always resolvingnullwith only a console warning.
61-80: Coordinate extraction is robust but could be tightened and deduplicatedThe multiple fallbacks for
lat/lngacross_geoloc,geoLocation, andlocationare helpful for handling legacy shapes, but:
- The repeated casts to
Record<string, unknown>could be wrapped in a small helper to avoid duplication.- If this is only intended for
Chapterobjects today, typing the parameter asChapter(or a narrower structural type) would give you better safety thanRecord<string, unknown>.
85-103: Distance sorting works, but the typing and filter could be more preciseThe map/filter/sort pipeline correctly excludes chapters without valid coordinates and orders the rest by distance. Two type‑level improvements would reduce assertions and make the return type easier to use:
- Narrow the filter with a type predicate instead of
item !== nullplusas/!.- Align the generic shape with actual usage (e.g.,
Chapter[]or a generic<T>that returnsArray<T & { distance: number }>), so you don’t lose the concrete type when calling this fromChapterMapWrapper.Example pattern:
.filter((item): item is { /* your shape */ } => item !== null)This will also help keep
sortedData’s type in sync with what’s returned.frontend/src/components/ChapterMapWrapper.tsx (2)
57-100: Avoid leakingshowLocationSharingprop intoChapterMapand alignsortedDatatypingTwo related refinements here:
ChapterMaplikely doesn’t declare ashowLocationSharingprop, but{...props}forwards it anyway. This can cause type errors ifChapterMaphas a strict props interface and also blurs the wrapper’s responsibility boundary.
sortedDatais typed asChapter[] | null, whilesortChaptersByDistancecurrently returns objects augmented with adistancefield. Even though this extra field is harmless at runtime, the mismatch can force casts or assertions elsewhere.You can address both by stripping the wrapper‑only prop and tightening the types:
-const ChapterMapWrapper: React.FC<ChapterMapWrapperProps> = (props) => { - const [userLocation, setUserLocation] = useState<UserLocation | null>(null) - const [isLoadingLocation, setIsLoadingLocation] = useState(false) - const [sortedData, setSortedData] = useState<Chapter[] | null>(null) +const ChapterMapWrapper: React.FC<ChapterMapWrapperProps> = (props) => { + const { showLocationSharing, ...mapProps } = props + const [userLocation, setUserLocation] = useState<UserLocation | null>(null) + const [isLoadingLocation, setIsLoadingLocation] = useState(false) + const [sortedData, setSortedData] = useState<Chapter[] | null>(null) // or Chapter & { distance: number }[] @@ - const enableLocationSharing = props.showLocationSharing === true + const enableLocationSharing = showLocationSharing === true @@ - setSortedData(sortChaptersByDistance(props.geoLocData, location)) + setSortedData(sortChaptersByDistance(mapProps.geoLocData, location)) @@ - const mapData = sortedData ?? props.geoLocData + const mapData = sortedData ?? mapProps.geoLocData @@ - <div className="space-y-4"> + <div className="space-y-4"> @@ - <ChapterMap {...props} geoLocData={mapData} /> + <ChapterMap {...mapProps} geoLocData={mapData} />Adjusting
sortChaptersByDistance’s return type as suggested ingeolocationUtils.tswill let you updatesortedData’s type accordingly.
60-97: Location‑sharing UI and accessibility are in good shape; consider a failure state messageThe button’s
aria-label,title, andisLoading/disabledflags make the interaction accessible, and the status text clearly indicates whether a location filter is active. One minor UX enhancement would be to surface a brief inline message when geolocation fails or is denied (instead of silently returning to the default “Find chapters near you” text), so users understand why nothing changed after clicking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/app/chapters/page.tsx(1 hunks)frontend/src/components/ChapterMapWrapper.tsx(1 hunks)frontend/src/utils/geolocationUtils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ChapterMapWrapper.tsx
🧬 Code graph analysis (1)
frontend/src/components/ChapterMapWrapper.tsx (2)
frontend/src/types/chapter.ts (1)
Chapter(4-21)frontend/src/utils/geolocationUtils.ts (3)
UserLocation(1-6)getUserLocationFromBrowser(31-59)sortChaptersByDistance(85-103)
🔇 Additional comments (3)
frontend/src/app/chapters/page.tsx (1)
90-102: Prop wiring for enabling location sharing looks correctPassing
showLocationSharing={true}here cleanly enables the new opt‑in location UI for the chapters page without affecting behavior when no chapters are loaded.frontend/src/utils/geolocationUtils.ts (1)
13-29: Haversine distance implementation looks mathematically soundThe
calculateDistanceimplementation (degree→radian conversion,aterm, andR = 6371km) is correct for a haversine distance in kilometers and handles edge cases (identical points) as expected.frontend/src/components/ChapterMapWrapper.tsx (1)
22-57: Location‑sharing flow and toggle behavior are logically soundThe wrapper cleanly gates the feature behind
showLocationSharing, toggles the filter on repeated clicks (set/unsetuserLocationandsortedData), and correctly guards loading state aroundgetUserLocationFromBrowserwith afinallyblock. This matches the opt‑in requirement without altering default behavior when the feature is disabled.
|
Could you please review this? Let me know if its looks fine so that i can proceed with test part |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 this works great! Please proceed with working on tests 👍🏼 This looks good so far! Feel free to move this back into drafts if needed.
I do have a couple of questions!
When I use the button the map is pretty zoomed out 🤔 For example, it shows the full state I'm in. Is there a way to zoom in more or is it the best we can get? Also a few comments down below ⬇️
| // # NOSONAR Geolocation permission is necessary for "Find chapters near you" feature. | ||
| // User explicitly opts-in via button click. Location stays client-side only. | ||
| // eslint-disable-next-line no-unsanitized/property | ||
| navigator.geolocation.getCurrentPosition( |
There was a problem hiding this comment.
I can see Sonar still gets triggered on this for some reason 🤔
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/src/components/ChapterMap.tsx (1)
109-148: Consider extracting duplicate logic.The userLocation block (lines 109-128) and showLocal block (lines 129-148) share nearly identical logic with only two differences: maxZoom value (12 vs 7) and the triggering condition. This duplication increases maintenance burden.
Consider extracting the shared logic:
const applyLocalZoom = (zoom: number) => { const maxNearestChapters = 5 const localChapters = validGeoLocData.slice(0, maxNearestChapters) const localBounds = L.latLngBounds( localChapters.map((chapter) => [ chapter._geoloc?.lat ?? chapter.geoLocation?.lat, chapter._geoloc?.lng ?? chapter.geoLocation?.lng, ]) ) const nearestChapter = validGeoLocData[0] map.setView( [ nearestChapter._geoloc?.lat ?? nearestChapter.geoLocation?.lat, nearestChapter._geoloc?.lng ?? nearestChapter.geoLocation?.lng, ], zoom ) map.fitBounds(localBounds, { maxZoom: zoom }) } if (userLocation && validGeoLocData.length > 0) { applyLocalZoom(12) } else if (showLocal && validGeoLocData.length > 0) { applyLocalZoom(7) }frontend/src/utils/geolocationUtils.ts (2)
31-60: Consider enabling high accuracy for "precise location" feature.The function uses
enableHighAccuracy: falsewhich may provide less accurate results. Since the PR title and feature description specifically mention "precise location sharing," consider setting this totruefor better accuracy, acknowledging it may take longer and use more battery.If you want to prioritize accuracy:
{ - enableHighAccuracy: false, + enableHighAccuracy: true, timeout: 10000, maximumAge: 0, }
86-104: Consider stronger typing for sortChaptersByDistance.The function uses
Record<string, unknown>[]instead of the more specificChapter[]type, which loses type safety and IDE support. Since the function is specifically designed to sort chapters and ChapterMapWrapper passesprops.geoLocData(typed asChapter[]), consider using the Chapter type directly.import type { Chapter } from 'types/chapter' export const sortChaptersByDistance = ( chapters: Chapter[], userLocation: UserLocation ): Array<Chapter & { distance: number }> => { return chapters .map((chapter) => { const { lat, lng } = extractChapterCoordinates(chapter as unknown as Record<string, unknown>) if (typeof lat !== 'number' || typeof lng !== 'number') return null const distance = calculateDistance(userLocation.latitude, userLocation.longitude, lat, lng) return { ...chapter, distance } }) .filter((item): item is Chapter & { distance: number } => item !== null) .sort((a, b) => a.distance - b.distance) }This eliminates the non-null assertions and provides better type safety.
frontend/src/components/ChapterMapWrapper.tsx (1)
50-52: Consider providing user feedback on location errors.The error handler logs to console but doesn't provide user feedback when location detection fails. Users who grant permission but encounter errors (timeout, position unavailable, etc.) won't know what happened.
Consider adding error state and displaying a user-friendly message:
const [error, setError] = useState<string | null>(null) // In handleShareLocation: try { setError(null) const location = await getUserLocationFromBrowser() if (!location) { setError('Unable to determine your location. Please try again.') return } // ... rest of logic } catch (error) { console.error('Error detecting location:', error) setError('Failed to access location. Please check your browser permissions.') } // In render: {error && ( <div className="text-xs text-red-600 dark:text-red-400">{error}</div> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/ChapterMap.tsx(4 hunks)frontend/src/components/ChapterMapWrapper.tsx(1 hunks)frontend/src/utils/geolocationUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/ChapterMap.tsx (2)
frontend/src/types/chapter.ts (1)
Chapter(4-21)frontend/src/utils/geolocationUtils.ts (1)
UserLocation(1-6)
frontend/src/components/ChapterMapWrapper.tsx (2)
frontend/src/types/chapter.ts (1)
Chapter(4-21)frontend/src/utils/geolocationUtils.ts (3)
UserLocation(1-6)getUserLocationFromBrowser(31-60)sortChaptersByDistance(86-104)
🔇 Additional comments (7)
frontend/src/components/ChapterMap.tsx (1)
5-5: LGTM!The UserLocation type import and optional prop addition are properly implemented with correct TypeScript typing.
Also applies to: 16-16, 21-21
frontend/src/utils/geolocationUtils.ts (2)
1-6: LGTM!Clean interface definition with appropriate required and optional fields.
13-29: LGTM!The Haversine formula implementation is correct for calculating great-circle distance between two points on a sphere.
frontend/src/components/ChapterMapWrapper.tsx (4)
1-20: LGTM!Clean imports and proper TypeScript typing for the component props and function component.
Also applies to: 22-22
23-31: LGTM!Clean state initialization and early return pattern when location sharing is disabled.
77-97: LGTM! Clean UI feedback states.The location status display provides clear feedback for both active and inactive states, with appropriate styling and accessibility attributes.
Note: I see from past review comments that displaying coordinates instead of city names was already discussed (would require reverse geocoding). The current coordinate display is a reasonable approach.
Based on learnings
58-58: LGTM! Proper data flow to ChapterMap.The component correctly selects between sorted and original data, and properly passes both the data and userLocation to ChapterMap for rendering.
Also applies to: 100-100
@anurag2787 It seems that Sonar is totally against it and only suggests to use user input and go from there :( I'm not sure if we can override this. @arkid15r is there anything in the SQ admin that we could disable for this approach to work? |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Shouldn't the new button be part of the map and available after a click on "Click to interact with map" button only? I believe it'd be cleaner.
We could just add a small button consistent with the map style (with a good tooltip). Instead of showing coordinates values we could just add a pin (the red one) indicating user's location on the map.
@kasya wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/utils/geolocationUtils.ts (2)
31-64: Harden browser-only usage and align// NOSONARwith the geolocation lineThe overall flow (opt‑in, local‑only usage, timeout, graceful error handling) looks solid. Two small tweaks would tighten things up and should help with Sonar:
- Guard against non‑browser environments (
typeof navigator === 'undefined') so this safely returnsnullinstead of throwing if ever called outside the browser.- Move
// NOSONARonto the line wherenavigator.geolocationis actually read, since Sonar only suppresses issues on the same line.Suggested refactor:
-export const getUserLocationFromBrowser = (): Promise<UserLocation | null> => { - return new Promise((resolve) => { - if (!navigator.geolocation) { - // eslint-disable-next-line no-console - console.warn('Geolocation API not supported') - resolve(null) - return - } +export const getUserLocationFromBrowser = (): Promise<UserLocation | null> => { + return new Promise((resolve) => { + if (typeof navigator === 'undefined' || !navigator.geolocation) { + // eslint-disable-next-line no-console + console.warn('Geolocation API not supported') + resolve(null) + return + } @@ - navigator.geolocation.getCurrentPosition( + const geolocation = navigator.geolocation // NOSONAR - geolocation used only after explicit user opt-in; data stays client-side (see comment above). + geolocation.getCurrentPosition( (position) => { resolve({ latitude: position.coords.latitude, longitude: position.coords.longitude, }) @@ - { - enableHighAccuracy: false, - timeout: 10000, - maximumAge: 0, - } - ) - }) // NOSONAR + { + enableHighAccuracy: false, + timeout: 10000, + maximumAge: 0, + } + ) + }) }Please re-run the Sonar check after this change to confirm the warning is actually suppressed on the targeted line.
80-101: Consider a small typing cleanup insortChaptersByDistanceLogic is correct, but you can make the API more ergonomic and type‑safe by preserving the caller’s chapter type and avoiding the
null+ type assertion combo:
- Make the function generic over the chapter type.
- Use a type‑predicate in the filter so you don’t need
a!and the final cast.Example refactor:
-export const sortChaptersByDistance = ( - chapters: Record<string, unknown>[], - userLocation: UserLocation -): Array<Record<string, unknown> & { distance: number }> => { - return chapters - .map((chapter) => { - const { lat, lng } = extractChapterCoordinates(chapter) - - if (typeof lat !== 'number' || typeof lng !== 'number') return null - - const distance = calculateDistance(userLocation.latitude, userLocation.longitude, lat, lng) - - return { ...chapter, distance } - }) - .filter((item) => item !== null) // remove invalid ones - .sort((a, b) => a!.distance - b!.distance) as Array< - Record<string, unknown> & { distance: number } - > -} +export const sortChaptersByDistance = <T extends Record<string, unknown>>( + chapters: T[], + userLocation: UserLocation +): Array<T & { distance: number }> => { + return chapters + .map((chapter) => { + const { lat, lng } = extractChapterCoordinates(chapter) + + if (typeof lat !== 'number' || typeof lng !== 'number') return null + + const distance = calculateDistance(userLocation.latitude, userLocation.longitude, lat, lng) + + return { ...chapter, distance } + }) + .filter((item): item is T & { distance: number } => item !== null) // remove invalid ones + .sort((a, b) => a.distance - b.distance) +}This keeps the original chapter shape at call sites while maintaining the same runtime behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/geolocationUtils.ts(1 hunks)
🔇 Additional comments (3)
frontend/src/utils/geolocationUtils.ts (3)
1-11: Location and chapter coordinate shapes look goodThe
UserLocationandChapterCoordinatesstructures match how this module uses them (lat/lng plus optional metadata), so there are no functional or safety concerns here from my side.
13-29: Haversine distance calculation is implemented correctlyThe formula, radian conversions, and choice of radius (Earth ≈ 6371 km) are all correct for computing great‑circle distance in kilometers. No changes needed.
66-78: Coordinate extraction matches current chapter data modelThis helper now cleanly reflects the actual
_geoloc/geoLocationshape (lat/lng only) and defers validity checks to the caller. That’s simple and effective; no further changes needed here.
|
The linked issue must be assigned to the PR author. |
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 please address Sonar issues that were introduced
As for your attached video in the issue - can we move the button to the bottom left corner and make it similar style as zoom buttons on the map?
This should be just a button with a target icon, no words. On hover we can say something like Share your current location to find nearby chapters
P.S. We reopened this, but in the future please do not abandon PRs like that. We have strict deadlines on issues, and it's not fair to other contributors if we allow others to just put PR in draft and keep it for weeks. If you need to take pause due to personal issues - just communicate it to us in advance.
|
Thanks for reopening the PR. I’ll update it soon according to the review and will keep this in mind from next time. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/ChapterMap.tsx (1)
138-175: Centering on nearest chapter works; fix off‑by‑one inshowLocalas previously notedThe new
userLocationbranch correctly:
- Uses the (distance‑sorted)
geoLocDatato picknearestChapter = validGeoLocData[0].- Fits bounds around up to
maxNearestChapterschapters viaslice(0, maxNearestChapters).However, the
showLocalbranch below still usesslice(0, maxNearestChapters - 1), so you only get 4 chapters whenmaxNearestChapters = 5. This is the same off‑by‑one issue flagged in earlier reviews and is still present.You can align both branches with:
- } else if (showLocal && validGeoLocData.length > 0) { - const maxNearestChapters = 5 - const localChapters = validGeoLocData.slice(0, maxNearestChapters - 1) + } else if (showLocal && validGeoLocData.length > 0) { + const maxNearestChapters = 5 + const localChapters = validGeoLocData.slice(0, maxNearestChapters)This ensures you consistently use the intended number of nearby chapters for bounds in both modes.
frontend/src/components/ChapterMapWrapper.tsx (1)
29-48: Fix distance-field destructuring in sorted data mapping
sortChaptersByDistanceaugments chapters with adistanceproperty, but this code destructures_distance, which doesn’t exist. As a result,distanceremains on the objects passed toChapterMap, which has already been reported to break tests and isn’t what you intend.Use destructuring to drop the real property name while avoiding ESLint’s unused-variable warning:
- if (location) { - setUserLocation(location) - const sorted = sortChaptersByDistance(props.geoLocData, location) - setSortedData(sorted.map(({ _distance, ...chapter }) => chapter as unknown as Chapter)) - } + if (location) { + setUserLocation(location) + const sorted = sortChaptersByDistance(props.geoLocData, location) + setSortedData( + sorted.map(({ distance: _, ...chapter }) => chapter as unknown as Chapter) + ) + }This removes
distancefrom the mapped objects, satisfying the tests and keeping the runtime shape aligned withChapter.
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
16-27: Prop additions are fine; consider simplifyinguserLocationnullabilityAdding optional
userLocationandonShareLocationkeeps existing call sites working and cleanly exposes the new behavior. Note thatuserLocation?: UserLocation | nullencodes three states (undefined/null/value); if you don’t need to distinguish undefined vs null, you could later simplify this to a single “missing” state to reduce mental overhead.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/img/user-marker-icon.pngis excluded by!**/*.png
📒 Files selected for processing (2)
frontend/src/components/ChapterMap.tsx(6 hunks)frontend/src/components/ChapterMapWrapper.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ChapterMap.tsxfrontend/src/components/ChapterMapWrapper.tsx
🧬 Code graph analysis (1)
frontend/src/components/ChapterMap.tsx (3)
frontend/src/types/chapter.ts (1)
Chapter(4-21)frontend/src/utils/geolocationUtils.ts (1)
UserLocation(1-6)frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (7)
frontend/src/components/ChapterMap.tsx (3)
2-5: New imports for location UI are consistent and scoped correctlyThe additional FontAwesome, HeroUI, and
UserLocationimports line up with how they’re used later in the file; no issues with tree‑shaking or type usage here.Also applies to: 9-9
183-203: Overlay pointer-events change correctly accommodates the new controlSwitching the overlay button to
pointer-events-nonewith apointer-events-autoinner<p>is a good way to:
- Keep the “Click to interact with map” CTA clickable.
- Avoid blocking interactions with the new location‑sharing button and the underlying map.
The behavior and accessibility of the activation overlay remain intact with the new control.
205-227: Location-sharing control wiring and accessibility look solidThe new bottom-left control is cleanly gated on
onShareLocation, uses clear tooltip/aria text for both “share” and “reset” states, and delegates behavior correctly via the callback instead of owning state here. No issues from a UX or data‑flow perspective.frontend/src/components/ChapterMapWrapper.tsx (4)
2-8: Geolocation utility imports are correct and type-safeUsing a single import for
getUserLocationFromBrowser,sortChaptersByDistance, andtype UserLocationkeeps the API surface tidy and leveragestype-only import for TS hygiene. Usage below matches these imports.
12-17: New props interface andshowLocationSharingflag are well-shapedDefining
ChapterMapWrapperPropswith an optionalshowLocationSharing?: booleanand later checking=== truegives a clear opt-in without impacting existing call sites. This matches the requirement to keep default behavior unchanged unless explicitly enabled.
19-27: Wrapper state and enable/disable path preserve existing behavior
userLocation/sortedDataare confined to this wrapper, and the early return whenenableLocationSharingis false ensures the oldChapterMapbehavior is preserved without needing changes elsewhere. This is a safe way to gate the new flow.
50-61: mapData selection and ChapterMap wiring look correct
mapData = sortedData ?? props.geoLocDatacleanly prefers distance-sorted chapters when available and falls back to the original data. PassinguserLocationandonShareLocation={handleShareLocation}through toChapterMapkeeps the concerns separated between data preparation and rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/utils/geolocationUtils.ts (1)
1-6: Address the unresolved past review comment about unused fields.The past review comment on these lines is still unresolved. The
cityandcountryfields are never populated bygetUserLocationFromBrowser(which only returnslatitudeandlongitude). Per the previous feedback, either remove these unused fields or add a clear comment explaining they are reserved for future enhancement (e.g., reverse geocoding).Apply this diff to add documentation if keeping them for future use:
export interface UserLocation { latitude: number longitude: number + // Reserved for future reverse-geocoding feature city?: string country?: string }Or remove them if not planned:
export interface UserLocation { latitude: number longitude: number - city?: string - country?: string }
🧹 Nitpick comments (1)
frontend/src/utils/geolocationUtils.ts (1)
57-61: Consider allowing brief location caching for better UX.Setting
maximumAge: 0forces a fresh location request every time, meaning users will be prompted again if they refresh the page. For a chapter-proximity feature, consider allowing brief caching (e.g.,maximumAge: 300000for 5 minutes) to improve the user experience without sacrificing accuracy.Apply this diff to enable brief caching:
{ enableHighAccuracy: false, timeout: 10000, - maximumAge: 0, + maximumAge: 300000, // Cache for 5 minutes }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/geolocationUtils.ts(1 hunks)
🔇 Additional comments (3)
frontend/src/utils/geolocationUtils.ts (3)
8-11: LGTM!The internal
ChapterCoordinatesinterface is clean and appropriately typed for the helper function.
13-29: LGTM!The Haversine formula implementation is mathematically correct and will accurately calculate great-circle distances between coordinates.
66-99: LGTM!Both
extractChapterCoordinatesandsortChaptersByDistanceare correctly implemented. The coordinate extraction properly handles the fallback chain, and the sorting logic correctly filters out invalid coordinates and orders chapters by proximity.
|
Hi @kasya i have updated the changes as per your review could you please take a look and let me know if everything looks good so that i can proceed with the writing test part |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/ChapterMap.tsx (2)
159-159: Fix pre-existing off-by-one error for consistency.Line 159 uses
slice(0, maxNearestChapters - 1), which returns only 4 chapters instead of the intended 5. The new user location path at line 140 correctly usesslice(0, maxNearestChapters). Fix this pre-existing bug for consistency:-const localChapters = validGeoLocData.slice(0, maxNearestChapters - 1) +const localChapters = validGeoLocData.slice(0, maxNearestChapters)
115-136: User location marker is never removed, causing stale/duplicate markers.Each effect run adds a new user marker without removing the previous one. When
userLocationchanges or is cleared via "Reset location filter", old markers remain on the map, causing accumulation.Track the user marker in a ref and remove it at the start of each effect run:
const mapRef = useRef<L.Map | null>(null) const markerClusterRef = useRef<MarkerClusterGroup | null>(null) +const userMarkerRef = useRef<L.Marker | null>(null) const [isMapActive, setIsMapActive] = useState(false)Then in the effect, before adding a new marker:
const map = mapRef.current +// Remove any existing user marker before re-adding or when location is cleared +if (userMarkerRef.current && map) { + map.removeLayer(userMarkerRef.current) + userMarkerRef.current = null +} + // Add user location marker if available if (userLocation && map) { ... userMarker.addTo(map) + userMarkerRef.current = userMarker }
📜 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 context used
🧠 Learnings (1)
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ChapterMap.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run spell check
- GitHub Check: Run frontend checks
- GitHub Check: Run pre-commit checks
🔇 Additional comments (4)
frontend/src/components/ChapterMap.tsx (4)
2-9: LGTM! Imports are appropriate for the new functionality.All new imports are necessary for the location-sharing feature and are from the correct packages.
20-27: LGTM! Props are correctly typed and appropriately optional.The optional nature of these props maintains backward compatibility while enabling the new location-sharing feature.
138-156: LGTM! Map centering logic for user location is correct.The logic properly centers on the nearest chapter and fits bounds using the first 5 chapters with appropriate zoom level.
177-177: LGTM! Dependencies correctly updated.Adding
userLocationto the dependency array ensures the effect re-runs when the location changes, which is necessary for updating the marker and map view.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 Thanks for updating the PR!
I pushed a few more changes. The styling of the button was not really matching with the existing zoom buttons styling (size, missing borders, border radius, etc).
Also the custom Pin image you added had watermarks all over it and did not match the pins we have on the map.
The tooltip was also misaligned with the button itself.
Please pay more attention to details next time 💯
I updated the code to use existing image of a pin that Leaflet uses and adjusted the color of it programmatically.
We also decided to move the button up to be aligned with zoom buttons.
I will approve this PR as it is right now, but I also created a separate issue for you to enhance current behavior of this button. All the requirements for that are listed in the issue 👌🏼 . The required updates are pretty small, but needed.
|
3 participants
Resolves #2583
Proposed change
This PR adds an opt-in precise location sharing option for the Chapter Map.
Checklist
make check-testlocally; all checks and tests passed.