Improves home page chapter map #1009
Conversation
Summary by CodeRabbit
WalkthroughThis pull request modifies the Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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: 3
🧹 Nitpick comments (2)
frontend/src/components/ChapterMap.tsx (2)
9-19: Consider adding a utility function for the Haversine formula.The Haversine formula implementation is correct, but it would be better to move this function to a utility file since it's a general geographic calculation that could be reused elsewhere in the application.
Move this function to a dedicated utility file:
// Create a new file: frontend/src/utils/geoUtils.ts + /** + * Calculates the distance between two geographic coordinates using the Haversine formula + * @param lat1 Latitude of first point in decimal degrees + * @param lng1 Longitude of first point in decimal degrees + * @param lat2 Latitude of second point in decimal degrees + * @param lng2 Longitude of second point in decimal degrees + * @returns Distance in kilometers + */ + export const getDistance = (lat1: number, lng1: number, lat2: number, lng2: number): number => { + const R = 6371 // Earth's radius in km + const dLat = ((lat2 - lat1) * Math.PI) / 180 + const dLng = ((lng2 - lng1) * Math.PI) / 180 + const a = + Math.sin(dLat / 2) * Math.sin(dLat / 2) + + Math.cos((lat1 * Math.PI) / 180) * Math.cos((lat2 * Math.PI) / 180) * + Math.sin(dLng / 2) * Math.sin(dLng / 2) + const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)) + return R * c + }Then in the ChapterMap component:
import L from 'leaflet' import React, { useEffect, useMemo, useRef } from 'react' import 'leaflet/dist/leaflet.css' import 'leaflet.markercluster/dist/MarkerCluster.css' import 'leaflet.markercluster/dist/MarkerCluster.Default.css' import 'leaflet.markercluster' import { GeoLocDataAlgolia, GeoLocDataGraphQL } from 'types/chapter' + import { getDistance } from 'utils/geoUtils' - const getDistance = (lat1: number, lng1: number, lat2: number, lng2: number) => { - const R = 6371 - const dLat = ((lat2 - lat1) * Math.PI) / 180 - const dLng = ((lng2 - lng1) * Math.PI) / 180 - const a = - Math.sin(dLat / 2) * Math.sin(dLat / 2) + - Math.cos((lat1 * Math.PI) / 180) * Math.cos((lat2 * Math.PI) / 180) * - Math.sin(dLng / 2) * Math.sin(dLng / 2) - const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)) - return R * c - }
93-115: Consider adding visual distinction for nearest chapters.While you're correctly processing the nearest chapters, they don't have any visual distinction on the map compared to other chapters.
Add visual distinction for the nearest chapters to improve user experience:
validChapters.forEach((chapter) => { + // Check if this is one of the nearest chapters + const isNearestChapter = userLocation && + nearestChapters.some(nc => nc.key === chapter.key); + + // Use a different icon for nearest chapters const markerIcon = new L.Icon({ iconAnchor: [12, 41], iconRetinaUrl: '/img/marker-icon-2x.png', iconSize: [25, 41], - iconUrl: '/img/marker-icon.png', + iconUrl: isNearestChapter ? '/img/marker-icon-nearest.png' : '/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 }) const popup = L.popup() const popupContent = document.createElement('div') popupContent.className = 'popup-content' - popupContent.textContent = chapter.name + + // Add distance information for nearest chapters + if (isNearestChapter && userLocation) { + const nearestChapter = nearestChapters.find(nc => nc.key === chapter.key); + const distance = nearestChapter?.distance?.toFixed(1) || 0; + popupContent.innerHTML = `<strong>${chapter.name}</strong><br>${distance} km from your location`; + } else { + popupContent.textContent = chapter.name; + }Note: You'll need to create a new marker icon file for nearest chapters or use a different approach for visual distinction.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ChapterMap.tsx(4 hunks)frontend/src/pages/Chapters.tsx(4 hunks)
🔇 Additional comments (5)
frontend/src/pages/Chapters.tsx (1)
113-113: LGTM! The userLocation prop is correctly passed to ChapterMap.The userLocation state is properly passed to the ChapterMap component, allowing it to use this information for chapter sorting and map display.
frontend/src/components/ChapterMap.tsx (4)
41-52: Excellent implementation of the nearest chapters calculation with useMemo.The implementation of nearestChapters is well done, using useMemo for performance optimization and properly handling the case when userLocation is null. The sorting by distance and limiting to the closest 5 chapters is a good approach.
82-91: Good validation of chapter coordinates.The thorough validation of chapter coordinates before processing them is excellent. This helps prevent mapping issues with invalid coordinates and improves the robustness of the application.
119-134: Well-implemented map bounds fitting with fallbacks.The bounds fitting logic with multiple fallbacks is well-implemented. The try-catch block provides good error handling for edge cases.
135-135: Don't forget to update dependency array for useEffect.You've correctly updated the dependency array to include the new
nearestChaptersanduserLocationvariables, ensuring the effect runs when these values change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/ChapterMap.tsx (3)
9-19: Good implementation of the Haversine formula for distance calculation.The
getDistancefunction correctly implements the Haversine formula to calculate the great-circle distance between two geographic points. This is the appropriate mathematical approach for determining the distance between coordinates on Earth's surface.Consider adding JSDoc comments to document this function's purpose, parameters, and return value:
+/** + * Calculates the distance between two geographical points using the Haversine formula + * @param lat1 - Latitude of the first point in decimal degrees + * @param lng1 - Longitude of the first point in decimal degrees + * @param lat2 - Latitude of the second point in decimal degrees + * @param lng2 - Longitude of the second point in decimal degrees + * @returns Distance between the points in kilometers + */ const getDistance = (lat1: number, lng1: number, lat2: number, lng2: number) => {
81-91: Consider extracting validChapters to a useMemo hook.The filtering of valid chapters is currently done inside the useEffect, but it only depends on
normalizedData. Moving this calculation into a useMemo hook would prevent unnecessary recalculation when other dependencies change.const normalizedData = useMemo(() => { return geoLocData.map((chapter) => ({ lat: '_geoloc' in chapter ? chapter._geoloc.lat : chapter.geoLocation.lat, lng: '_geoloc' in chapter ? chapter._geoloc.lng : chapter.geoLocation.lng, key: chapter.key, name: chapter.name, })) }, [geoLocData]) + const validChapters = useMemo(() => { + return normalizedData.filter(chapter => + chapter.lat !== null && + chapter.lng !== null && + !isNaN(chapter.lat) && + !isNaN(chapter.lng) && + chapter.lat >= -90 && + chapter.lat <= 90 && + chapter.lng >= -180 && + chapter.lng <= 180 + ) + }, [normalizedData]) useEffect(() => { // ...existing code... const markerClusterGroup = L.markerClusterGroup() const bounds: [number, number][] = [] - // Validate and filter out invalid coordinates - const validChapters = normalizedData.filter(chapter => - chapter.lat !== null && - chapter.lng !== null && - !isNaN(chapter.lat) && - !isNaN(chapter.lng) && - chapter.lat >= -90 && - chapter.lat <= 90 && - chapter.lng >= -180 && - chapter.lng <= 180 - ) validChapters.forEach((chapter) => { // ...existing code... }) // ...rest of the effect... -}, [normalizedData, nearestChapters, userLocation]) +}, [validChapters, nearestChapters, userLocation])Also applies to: 93-115
41-52: Consider adding a configurable limit for nearest chapters.The nearest chapters are currently hard-limited to 5. Consider making this a configurable prop to allow more flexibility in different contexts.
const ChapterMap = ({ geoLocData, userLocation, style, + nearestLimit = 5, }: { geoLocData: GeoLocDataGraphQL[] | GeoLocDataAlgolia[] userLocation: { lat: number; lng: number } | null style: React.CSSProperties + nearestLimit?: number }) => { // ...existing code... const nearestChapters = useMemo(() => { if (!userLocation) return normalizedData // Sort chapters by distance from user location return normalizedData .map((chapter) => ({ ...chapter, distance: getDistance(userLocation.lat, userLocation.lng, chapter.lat, chapter.lng), })) .sort((a, b) => a.distance - b.distance) - .slice(0, 5) + .slice(0, nearestLimit) }, [userLocation, normalizedData, nearestLimit]) // ...rest of component... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(4 hunks)
🔇 Additional comments (10)
frontend/src/components/ChapterMap.tsx (10)
21-29: Props interface update correctly extends functionality.The addition of the
userLocationprop with proper typing allows the component to receive the user's geographical coordinates, enabling the proximity-based sorting feature.
41-52: Well-implemented nearest chapters calculation with useMemo.The useMemo hook efficiently calculates the nearest chapters based on user location, with proper handling of the case when user location is unavailable. The logic correctly:
- Maps over chapters to calculate distances
- Sorts by increasing distance
- Limits results to the 5 nearest chapters
This optimizes performance by only recalculating when dependencies change.
57-58: Map initialization parameter adjustment.Setting
worldCopyJumpto false prevents the map from wrapping around the world when panning, which is generally appropriate for this use case.
59-61: Map bounds correctly defined.The maxBounds property is correctly set to encompass the entire world with proper latitude (-90 to 90) and longitude (-180 to 180) ranges.
81-91: Excellent implementation of coordinate validation.The validation logic thoroughly checks for invalid coordinates by ensuring values:
- Are not null
- Are not NaN
- Fall within valid latitude (-90 to 90) and longitude (-180 to 180) ranges
This prevents map rendering issues and improves robustness against bad data.
93-93: Properly updated to use validated chapters.The code now correctly iterates only over chapters with valid coordinates.
95-102: Icon configuration improvement.The marker icon configuration has been refined with properly defined dimensions and anchor points.
119-134: Robust bounds fitting with appropriate fallback mechanisms.The enhanced bounds fitting logic provides a user-focused experience by:
- Prioritizing nearest chapters when user location is available
- Falling back to all chapters if nearest chapters can't be used
- Defaulting to a preset view if all else fails
The try-catch block properly handles potential errors that might occur during bounds fitting.
135-135: Updated useEffect dependencies correctly reflect new data dependencies.The useEffect hook now properly depends on
nearestChaptersanduserLocationin addition tonormalizedData, ensuring it runs when any of these values change.
137-137: Simplified return statement.The return statement has been cleaned up for better readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/pages/Chapters.tsx (1)
111-111: Consider providing a loading state indication for geolocation.The
userLocationprop is passed to the ChapterMap component without any indication of whether the location is being fetched or if there was an error. This can lead to confusion for users who may be waiting for the map to center on their location.Add a loading state indication:
<ChapterMap geoLocData={searchQuery ? chapters : geoLocData} - userLocation={userLocation} + userLocation={userLocation} + isLoadingLocation={isRequestingLocation} + locationError={locationError} style={{ height: '400px', width: '100%', zIndex: '0' }} />Then update the ChapterMap component to handle these props and display appropriate UI elements.
frontend/src/components/ChapterMap.tsx (3)
9-21: Enhance distance calculation with type validation and constant documentation.The Haversine formula implementation looks good, but could benefit from additional documentation and type validation:
const getDistance = (lat1: number, lng1: number, lat2: number, lng2: number) => { - const R = 6371 + // Earth radius in kilometers + const R = 6371 + // Convert degrees to radians const dLat = ((lat2 - lat1) * Math.PI) / 180 const dLng = ((lng2 - lng1) * Math.PI) / 180 + // Haversine formula calculation const a = Math.sin(dLat / 2) * Math.sin(dLat / 2) + Math.cos((lat1 * Math.PI) / 180) * Math.cos((lat2 * Math.PI) / 180) * Math.sin(dLng / 2) * Math.sin(dLng / 2) const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)) return R * c }
43-54: Make the number of nearest chapters configurable and add distance display.The current implementation hardcodes the number of nearest chapters to 5. Consider making this configurable and showing the distance to the user:
const nearestChapters = useMemo(() => { if (!userLocation) return normalizedData + // Number of nearest chapters to display + const maxNearestChapters = 5 // Sort chapters by distance from user location return normalizedData .map((chapter) => ({ ...chapter, distance: getDistance(userLocation.lat, userLocation.lng, chapter.lat, chapter.lng), })) .sort((a, b) => a.distance - b.distance) - .slice(0, 5) + .slice(0, maxNearestChapters) }, [userLocation, normalizedData])Also consider exposing the distance in the popup to provide users with valuable information:
// Inside the forEach loop where popups are created const popupContent = document.createElement('div') popupContent.className = 'popup-content' - popupContent.textContent = chapter.name + popupContent.innerHTML = ` + <strong>${chapter.name}</strong> + ${chapter.distance !== undefined ? `<p>Distance: ${Math.round(chapter.distance)} km</p>` : ''} + `
84-94: Add logging for invalid coordinates to help with data quality.The validation logic for filtering out invalid coordinates is excellent. To further improve data quality, consider adding logging for invalid coordinates that are filtered out:
// Validate and filter out invalid coordinates const validChapters = normalizedData.filter( (chapter) => { + const isValid = chapter.lat !== null && chapter.lng !== null && !isNaN(chapter.lat) && !isNaN(chapter.lng) && chapter.lat >= -90 && chapter.lat <= 90 && chapter.lng >= -180 && chapter.lng <= 180 + + if (!isValid) { + console.warn(`Invalid coordinates for chapter ${chapter.name}: lat=${chapter.lat}, lng=${chapter.lng}`) + } + + return isValid } )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ChapterMap.tsx(4 hunks)frontend/src/pages/Chapters.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (4)
frontend/src/pages/Chapters.tsx (3)
17-17: Add location consent mechanism and error handling.While adding the user location state is a good addition, it's important to address privacy and user experience concerns. Currently, the application requests the user's location without explaining why or handling potential errors.
Enhance the user location handling by adding error state and user consent:
- const [userLocation, setUserLocation] = useState<{ lat: number; lng: number } | null>(null) + const [userLocation, setUserLocation] = useState<{ lat: number; lng: number } | null>(null) + const [locationError, setLocationError] = useState<string | null>(null) + const [locationConsent, setLocationConsent] = useState<boolean>(false)
50-59: Implement error handling and user feedback for geolocation.The current implementation doesn't handle geolocation errors, which can occur if the user denies permission or if the browser can't determine location. Additionally, there's no indication to users why their location is being requested.
Add error handling and improve the user experience:
const fetchUserLocation = () => { - if (navigator.geolocation) { + if (navigator.geolocation && locationConsent) { navigator.geolocation.getCurrentPosition( (position) => { setUserLocation({ lat: position.coords.latitude, lng: position.coords.longitude, }) + setLocationError(null) }, + (error) => { + setLocationError( + error.code === 1 + ? 'Location permission denied. Chapters will not be sorted by proximity.' + : 'Unable to retrieve your location. Chapters will not be sorted by proximity.' + ) + console.error('Geolocation error:', error) + }, + { timeout: 10000, maximumAge: 60000 } ) + } else if (!navigator.geolocation) { + setLocationError('Geolocation is not supported by your browser. Chapters will not be sorted by proximity.') } }
62-62: Add conditional location fetching based on user consent.Rather than automatically fetching the user's location on component mount, it's better to request user consent first due to privacy considerations.
Update the implementation to request user consent before fetching location:
fetchData() - fetchUserLocation() + // Location will be fetched after user provides consent through a UI controlThen add a consent UI in the return statement:
{!locationConsent && !locationError && ( <div className="geolocation-consent"> <p>Enable location services to find chapters near you?</p> <button onClick={() => { setLocationConsent(true); fetchUserLocation(); }} > Allow </button> <button onClick={() => setLocationError('Location access declined.')}> Decline </button> </div> )} {locationError && ( <div className="geolocation-error">{locationError}</div> )}frontend/src/components/ChapterMap.tsx (1)
123-139: Well-structured fallback mechanism for map bounds.The implementation of a fallback mechanism for fitting map bounds is well-structured and handles various scenarios gracefully:
- If user location is available, it attempts to fit the bounds to the nearest chapters
- If that fails, it falls back to fitting the bounds of all valid chapters
- If all else fails, it defaults to a preset view
This ensures a good user experience in all scenarios. Great job!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/src/pages/Chapters.tsx (3)
17-17: 🛠️ Refactor suggestionAdd location consent mechanism
Adding user location tracking without explicit consent violates privacy best practices. This should be combined with a user consent mechanism and proper error handling.
Follow the previous review suggestion to add location consent:
+ const [locationConsent, setLocationConsent] = useState<boolean>(false) + const [locationError, setLocationError] = useState<string | null>(null) const [userLocation, setUserLocation] = useState<{ lat: number; lng: number } | null>(null)
54-68: 🛠️ Refactor suggestionImplement comprehensive geolocation error handling
The current implementation catches errors but doesn't provide user feedback or handle location permission denials appropriately.
Implement proper error handling as previously suggested:
const fetchUserLocation = () => { - if (navigator.geolocation) { + if (navigator.geolocation && locationConsent) { navigator.geolocation.getCurrentPosition( (position) => { setUserLocation({ lat: position.coords.latitude, lng: position.coords.longitude, }) + setLocationError(null) }, (error) => { - console.warn('Geolocation error:', error) + setLocationError( + error.code === 1 + ? 'Location permission denied. Chapters will not be sorted by proximity.' + : 'Unable to retrieve your location. Chapters will not be sorted by proximity.' + ) + console.error('Geolocation error:', error) }, + { timeout: 10000, maximumAge: 60000 } ) + } else if (!navigator.geolocation) { + setLocationError('Geolocation is not supported by your browser. Chapters will not be sorted by proximity.') } }🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 64-64: ESLint: Unexpected console statement (no-console)
70-71: 🛠️ Refactor suggestionRequest user consent before fetching location
Automatically fetching user location on component mount violates privacy best practices. This should only happen after explicit user consent.
fetchChapterData() - fetchUserLocation() + // Location will be fetched after user provides consent through a UI controlAnd add a consent UI component in the return statement:
{!locationConsent && !locationError && ( <div className="geolocation-consent"> <p>Enable location services to find chapters near you?</p> <button onClick={() => { setLocationConsent(true); fetchUserLocation(); }} > Allow </button> <button onClick={() => setLocationError('Location access declined.')}> Decline </button> </div> )} {locationError && ( <div className="geolocation-error">{locationError}</div> )}
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
9-21: Improve Haversine distance calculation function readabilityThe getDistance function correctly implements the Haversine formula but could be improved for readability and maintainability.
const getDistance = (lat1: number, lng1: number, lat2: number, lng2: number) => { - const R = 6371 + // Earth's radius in kilometers + const EARTH_RADIUS_KM = 6371 + + // Convert latitude and longitude from degrees to radians + const lat1Rad = (lat1 * Math.PI) / 180 + const lat2Rad = (lat2 * Math.PI) / 180 + const latDiffRad = ((lat2 - lat1) * Math.PI) / 180 + const lngDiffRad = ((lng2 - lng1) * Math.PI) / 180 - const dLat = ((lat2 - lat1) * Math.PI) / 180 - const dLng = ((lng2 - lng1) * Math.PI) / 180 const a = - Math.sin(dLat / 2) * Math.sin(dLat / 2) + - Math.cos((lat1 * Math.PI) / 180) * - Math.cos((lat2 * Math.PI) / 180) * - Math.sin(dLng / 2) * - Math.sin(dLng / 2) + Math.sin(latDiffRad / 2) * Math.sin(latDiffRad / 2) + + Math.cos(lat1Rad) * Math.cos(lat2Rad) * Math.sin(lngDiffRad / 2) * Math.sin(lngDiffRad / 2) const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)) - return R * c + return EARTH_RADIUS_KM * c }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ChapterMap.tsx(3 hunks)frontend/src/pages/Chapters.tsx(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/src/pages/Chapters.tsx
[error] 50-50: ESLint: Unexpected console statement (no-console)
[error] 64-64: ESLint: Unexpected console statement (no-console)
🔇 Additional comments (6)
frontend/src/pages/Chapters.tsx (2)
106-108: Good use of useMemo for performance optimizationUsing useMemo for mapData is a good practice as it avoids unnecessary recalculations when dependencies don't change.
123-127: Good conditional rendering with proper propsThe conditional rendering of ChapterMap and passing of the relevant props is implemented correctly.
frontend/src/components/ChapterMap.tsx (4)
24-30: Properly defined props with clear typingThe props for the ChapterMap component are well defined with appropriate types and default values.
44-54: Well implemented nearest chapters calculationThe nearestChapters calculation correctly uses the distance function, handles the null case gracefully, and uses useMemo for performance optimization.
84-94: Robust coordinate validationThe coordinate validation is thorough and prevents errors from invalid data. It checks for null values, NaN, and ensures coordinates are within valid geographical ranges.
124-141: Excellent progressive fallback strategy for map boundsThe implementation uses a smart progressive fallback strategy:
- First tries to focus on nearest chapters if user location is available
- Falls back to all chapters if nearest chapters aren't available
- Defaults to a global view if bounds fitting fails
This ensures a good user experience in all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/ChapterMap.tsx (3)
9-21: Excellent implementation of the Haversine formula for distance calculation.The
getDistancefunction correctly implements the Haversine formula to calculate the great-circle distance between two geographical points. The implementation uses the proper mathematical constants and transformations to compute distances in kilometers.Consider adding a simple JSDoc comment above the function to explain that it calculates the distance in kilometers and uses the Haversine formula. This would improve documentation for future developers working with this code.
57-61: Consider documenting the worldCopyJump property decision.The
worldCopyJumpproperty is set tofalse, which prevents the map from wrapping around the dateline. This is a reasonable choice for a chapter map with specific locations, but it might be worth adding a brief comment explaining this decision, especially if it represents a change from previous behavior.
126-144: Excellent map view management with proper fallback mechanism.The map view determination follows a smart prioritization approach:
- First tries to display nearest chapters when user location is available
- Falls back to showing all chapters if no user location
- Provides a sensible default view if bounds fitting fails
This creates a good user experience with graceful degradation.
Consider extracting the bounds fitting logic to a separate function to improve readability, something like:
const fitMapToAppropriateView = (map: L.Map) => { try { if (userLocation && nearestChapters.length > 0) { const nearestBounds = nearestChapters.map( (chapter) => [chapter.lat, chapter.lng] as [number, number] ) map.fitBounds(nearestBounds, { maxZoom: 10, padding: [50, 50], }) } else if (bounds.length > 0) { map.fitBounds(bounds) } } catch { map.setView([20, 0], 2) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (8)
frontend/src/components/ChapterMap.tsx (8)
23-30: Properly typed prop addition for user location.The addition of the
userLocationoptional prop with appropriate TypeScript typing is well-implemented. This change enables the component to handle geolocation-based features.
44-54: Well-implemented nearest chapters calculation with proper memoization.The
nearestChaptersimplementation efficiently:
- Handles the null/undefined case for userLocation
- Calculates distances only when necessary
- Sorts chapters by proximity
- Limits to 5 nearest chapters for better user experience
The
useMemohook correctly specifies dependencies to prevent unnecessary recalculations.
62-64: Map bounds setting is properly configured.The maxBounds setting correctly defines the entire globe as the boundary (latitude -90 to 90, longitude -180 to 180).
75-83: Good cleanup pattern for marker cluster group.The code properly removes any existing marker cluster group before creating a new one, which prevents accumulation of markers and potential memory leaks or visual artifacts.
86-97: Robust validation for chapter coordinates.The validation logic thoroughly checks for:
- Null values
- NaN values
- Valid latitude range (-90 to 90)
- Valid longitude range (-180 to 180)
This prevents potential rendering issues or errors from invalid geographical data.
99-122: Well-structured marker creation with appropriate configuration.The marker creation logic is well-implemented with proper icon configuration and event handling. The addition of markers only for valid chapters ensures map stability.
145-145: Correctly updated useEffect dependencies.The useEffect dependency array properly includes
nearestChaptersanduserLocationsince these values are now used within the effect. This ensures the effect runs when these values change.
147-147: Clean component rendering.The component rendering is clean and straightforward, maintaining a simple structure for the map container.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/components/ChapterMap.tsx (5)
55-56: Ensure proper cleanup for the marker cluster reference.
When the component unmounts, consider removing layers or resettingmarkerClusterRef.currentto avoid potential stale references.
57-68: Robust coordinate validation approach.
Filtering out invalid or out-of-bounds lat/lng values before rendering markers improves stability and prevents runtime errors. You could refactor this filter into a small helper function if it’s needed elsewhere for improved reusability.
99-111: Consider generalizing fixed “6th chapter” logic.
Targeting only the 6th chapter and fitting bounds around the first 6 might be a strict requirement, but if this logic changes in the future (e.g., highlight the 10th, 20th, etc.), you could introduce a configurable parameter or an auto-center approach.
120-122: Consider user-facing error handling.
Catching and logging errors is great, but you might also consider informing users when the map cannot render properly, instead of silently defaulting the view.
126-126: Return statement with static ID.
Returning a single<div id="chapter-map"/>is fine if only one map is used on each page. If multiple maps might appear later, consider unique IDs or dynamic approach.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ChapterMap.tsx(3 hunks)frontend/src/pages/Chapters.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/pages/Chapters.tsx
🔇 Additional comments (6)
frontend/src/components/ChapterMap.tsx (6)
17-17: Good addition ofmarkerClusterReffor improved cluster management.
This reference allows you to conveniently manage marker clusters and access cluster-specific methods. Ensure that you reset or clean up the cluster group on component unmount if needed to prevent memory leaks.
31-34: Check whetherworldCopyJump: falsealigns with your desired user map experience.
By settingmaxBoundsto the entire globe and disablingworldCopyJump, the map will not horizontally wrap. This prevents users from smoothly panning across the 180° meridian boundary, which may or may not be preferable depending on the project's requirements.
52-52: Informative comment.
The explanatory comment “Create a new marker cluster group” is clear, concise, and helpful for maintainers.
70-81: Marker creation logic is solid, with well-structured icon configuration.
Using a custom icon and binding popups individually is clear. Verify that the icon asset paths (/img/marker-icon.png, etc.) are correct in production builds.
97-98: Clear documentation.
Writing a concise comment (“Determine map view based on 6th index”) keeps future maintainers informed of your logic.
112-119: Well-structured fallback logic for fewer than six chapters.
Gradually narrowing down through different scenarios (>=6, >0, etc.) is readable. Ensure testing covers each branch, especially edge cases with no valid chapters.
|
Resolves #928