Fix ChapterMap locking behavior and wide screen layout issues #3322
Conversation
WalkthroughAdds dynamic minZoom and expanded bounds behavior, enables/disables map interactions (dragging, touchZoom, doubleClickZoom, keyboard) based on lock state with cleanup, updates unlock UI for keyboard activation, and extends test mocks to simulate container size and interaction APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In @frontend/src/components/ChapterMap.tsx:
- Around line 78-84: Guard against division by zero when computing aspectRatio:
check container.clientHeight (from container = map.getContainer()) before
dividing; if clientHeight is 0 or falsy, choose a safe fallback (e.g., treat
aspectRatio as <=2 so dynamicMinZoom becomes 2, or explicitly set dynamicMinZoom
to 2) and then call map.setMinZoom(dynamicMinZoom); ensure you reference
container.clientHeight, aspectRatio, dynamicMinZoom and map.setMinZoom when
adding the defensive check.
- Around line 194-201: The current maxBounds declaration uses Infinity and
-Infinity which produces invalid LatLngBounds; update the maxBounds passed to
the map (the prop named maxBounds in ChapterMap component) to use finite
longitude values (e.g., use [-180, 180]) such as replacing [[-85, -Infinity],
[85, Infinity]] with [[-85, -180], [85, 180]] or implement a wrapLatLngBounds
helper and apply it to maxBounds when worldCopyJump behavior is needed; ensure
maxBoundsViscosity and worldCopyJump remain unchanged and that the updated
bounds are valid LatLngBounds for Leaflet.
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
22-37: Mocks added for interaction handlers, but assertions are missing in tests.The mocks are correctly structured and comprehensive. However, the existing tests only verify
scrollWheelZoom.enable/disableis called (lines 217, 231, 341, 354), but don't assert that the newly added interaction handlers (dragging,touchZoom,doubleClickZoom,keyboard) are enabled/disabled appropriately.Consider extending the assertions in the "unlocks map and shows zoom controls on click" and "locks map again on mouse leave" tests:
await waitFor(() => { expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled() + expect(mockMap.dragging.enable).toHaveBeenCalled() + expect(mockMap.touchZoom.enable).toHaveBeenCalled() + expect(mockMap.doubleClickZoom.enable).toHaveBeenCalled() + expect(mockMap.keyboard.enable).toHaveBeenCalled() expect(L.control.zoom).toHaveBeenCalled() })frontend/src/components/ChapterMap.tsx (1)
124-124: Clarify the intent ofMath.max(dynamicMinZoom, 2).When
dynamicMinZoomis1(for very wide screens), this still sets the initial view zoom to2. This seems intentional — the user can manually zoom out to level 1, but the default world view starts at zoom 2.If this is the intended behavior, consider adding a brief comment to clarify:
-map.setView([20, 0], Math.max(dynamicMinZoom, 2)) +// Start at zoom 2 minimum for world view; user can zoom out further on wide screens +map.setView([20, 0], Math.max(dynamicMinZoom, 2))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsxfrontend/src/components/ChapterMap.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/ChapterMap.test.tsx
🔇 Additional comments (6)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
13-17: LGTM! Mocks correctly support the new dynamic minZoom behavior.The
setMinZoomandgetContainermocks align well with the new container-aware zoom calculations inChapterMap.tsx.frontend/src/components/ChapterMap.tsx (5)
25-28: LGTM! Comprehensive interaction enabling when map is active.This correctly addresses the PR objective of ensuring all interactions are enabled upon unlock.
36-39: LGTM! All interactions properly disabled when map is locked.This enforces the locking behavior as specified in issue #3012 — all interactions (dragging, zooming, keyboard navigation) are disabled until explicitly unlocked.
52-55: LGTM! Cleanup effect correctly disables all interactions on unmount.Good defensive cleanup to ensure no stale interaction handlers remain.
101-102: LGTM! Padding improves bounds fitting on wide screens.Adding consistent padding prevents markers from being clipped at viewport edges.
113-122: LGTM! Consistent padding applied for local chapter view.Good consistency with the userLocation branch above.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/src/components/ChapterMap.tsx:
- Around line 254-275: The button currently has "pointer-events-none" which
blocks clicks/keyboard activating its handlers (setIsMapActive) while the inner
<p> has "pointer-events-auto"; move the pointer-events behavior to the clickable
element by removing "pointer-events-none" from the <button> and adding
"pointer-events-auto" to the <button>'s class (and remove it from the inner <p>
if present), keeping the existing onClick/onKeyDown/tabIndex/aria-label so
setIsMapActive fires correctly.
🧹 Nitpick comments (4)
frontend/src/components/ChapterMap.tsx (4)
115-122: RedundantsetViewcall beforefitBounds.
map.setView()on lines 115-121 is immediately overridden bymap.fitBounds()on line 122. ThefitBoundscall will reposition and zoom the map, making the precedingsetViewineffective.♻️ Suggested fix: Remove the redundant setView call
const maxZoom = 7 const padding = 50 - const nearestChapter = validGeoLocData[0] - map.setView( - [ - nearestChapter._geoloc?.lat ?? nearestChapter.geoLocation?.lat, - nearestChapter._geoloc?.lng ?? nearestChapter.geoLocation?.lng, - ], - maxZoom - ) map.fitBounds(localBounds, { maxZoom: maxZoom, padding: [padding, padding] })
78-84: Consider adding a resize observer for dynamic minZoom recalculation.The dynamic minZoom is computed from container dimensions, but the effect only runs when
userLocation,showLocal, orvalidGeoLocDatachange. If the viewport is resized (e.g., window resize, orientation change), the minZoom won't update until another dependency changes.This may be acceptable for the current use case, but consider adding a resize observer if responsive behavior is important.
169-179: Minor: Extra whitespace indivIconHTML string.The HTML string contains extraneous spaces at the start (
' <div) and around the closing tags (/> </div> '). While this doesn't affect functionality, it could be cleaned up for consistency.♻️ Suggested cleanup
L.divIcon({ - html: ' <div aria-label="User location" role="img"><img src="/img/marker-icon.png" style="filter: hue-rotate(150deg) saturate(1.5) brightness(0.9); width: 25px; height: 41px;" alt="" aria-hidden="true" /> </div> ', + html: '<div aria-label="User location" role="img"><img src="/img/marker-icon.png" style="filter: hue-rotate(150deg) saturate(1.5) brightness(0.9); width: 25px; height: 41px;" alt="" aria-hidden="true" /></div>', className: 'user-location-marker',
217-240: MoveuseMemoto component body for better readability.Calling
useMemoinside JSX (within the render return) is unconventional. While React can handle this since the hook is called in a consistent order, extracting this memoized value to the top of the component body improves readability and follows common patterns.♻️ Suggested refactor
Move the memoized markers to the component body:
// Add near line 152, after validGeoLocData memo const markers = useMemo( () => validGeoLocData.map((chapter) => ( <Marker key={chapter.key} position={[ chapter._geoloc?.lat ?? chapter.geoLocation?.lat, chapter._geoloc?.lng ?? chapter.geoLocation?.lng, ]} icon={chapterIcon} > <Popup> <button type="button" className="cursor-pointer font-medium hover:text-blue-600" onClick={() => router.push(`/chapters/${chapter.key}`)} > {chapter.name} </button> </Popup> </Marker> )), [validGeoLocData, chapterIcon, router] ) // Then in JSX: <MarkerClusterGroup chunkedLoading> {markers} </MarkerClusterGroup>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChapterMap.tsx
🔇 Additional comments (2)
frontend/src/components/ChapterMap.tsx (2)
18-63: LGTM! Comprehensive interaction control.The
MapZoomControlcomponent properly enables/disables all map interaction handlers (dragging, touchZoom, doubleClickZoom, keyboard, scrollWheelZoom) based onisMapActivestate, and the cleanup effect ensures interactions are disabled on unmount. This aligns well with the PR objective of enforcing complete map locking.
197-201: No changes needed. Using[-Infinity, Infinity]for longitude bounds inmaxBoundsis a valid and established pattern in Leaflet, supported by community examples and working implementations. Leaflet correctly handlesInfinityvalues for effectively removing horizontal panning restrictions while keeping latitude bounded to[-85, 85]. This is intentional and correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/ChapterMap.tsx (2)
78-84: Dynamic minZoom won't update on window resize.The dynamic minZoom is calculated once when dependencies change, but if the user resizes the browser window, the container dimensions change without triggering a recalculation. This could lead to incorrect minZoom on wide screens after resize.
Consider adding a resize observer or window resize listener if dynamic minZoom should adapt to viewport changes:
♻️ Optional enhancement for resize handling
useEffect(() => { if (!map) return const updateMinZoom = () => { const container = map.getContainer() const width = container.clientWidth const height = container.clientHeight const aspectRatio = height > 0 ? width / height : 1 const dynamicMinZoom = aspectRatio > 2 ? 1 : 2 map.setMinZoom(dynamicMinZoom) } updateMinZoom() window.addEventListener('resize', updateMinZoom) return () => window.removeEventListener('resize', updateMinZoom) }, [map])
255-272: Redundant tabIndex and onKeyDown on native button.Native
<button>elements are already focusable (tabIndex is implicit) and automatically respond to Enter and Space key presses. The explicittabIndex={0}andonKeyDownhandler are unnecessary.♻️ Simplified button without redundant attributes
<button type="button" - tabIndex={0} className="pointer-events-auto flex cursor-pointer items-center justify-center rounded-md bg-white/90 px-5 py-3 text-sm font-medium text-gray-700 shadow-lg transition-colors hover:bg-gray-200 hover:text-gray-900 dark:bg-gray-700 dark:text-white dark:hover:bg-gray-600 dark:hover:text-white" onClick={() => { setIsMapActive(true) }} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - setIsMapActive(true) - } - }} aria-label="Unlock map" >frontend/__tests__/a11y/components/ChapterMap.a11y.test.tsx (1)
166-177: Consider adding keyboard unlock test for accessibility coverage.While the unlock button click is tested, accessibility testing would benefit from verifying that keyboard-only users can unlock the map using Enter or Space keys.
🧪 Optional: Add keyboard unlock test
it('should unlock map via keyboard (Enter key)', async () => { const { baseElement } = render(<ChapterMap {...defaultProps} onShareLocation={jest.fn()} />) const unlockButton = screen.getByLabelText('Unlock map') fireEvent.keyDown(unlockButton, { key: 'Enter', code: 'Enter' }) await waitFor(() => expect(screen.getByLabelText(/Share location/i)).toBeInTheDocument()) const results = await axe(baseElement) expect(results).toHaveNoViolations() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/a11y/components/ChapterMap.a11y.test.tsxfrontend/src/components/ChapterMap.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ChapterMap.tsx
🔇 Additional comments (6)
frontend/src/components/ChapterMap.tsx (5)
25-29: LGTM! Proper interaction toggle based on lock state.The symmetric enabling/disabling of dragging, touchZoom, doubleClickZoom, and keyboard interactions correctly implements the locking behavior per the PR objectives.
Also applies to: 36-40
48-61: LGTM! Proper cleanup on unmount.The cleanup effect correctly disables all map interactions when the component unmounts, preventing potential state issues.
101-102: LGTM! Consistent padding applied to fitBounds.The padding ensures markers aren't placed at the very edge of the viewport, improving the user experience.
123-125: LGTM! Default view respects minimum zoom constraint.Using
Math.max(dynamicMinZoom, 2)ensures a reasonable default zoom level while still allowing the map to zoom out further on wide screens if needed.
194-201: LGTM! Map configuration optimized for wide screens.Good choices:
worldCopyJump={true}prevents tile repetition when panning- Bounds at ±85° latitude correctly clips the extreme polar regions where Web Mercator distortion is severe
- Reduced viscosity (0.5) provides a smoother panning experience
frontend/__tests__/a11y/components/ChapterMap.a11y.test.tsx (1)
17-37: LGTM! Mocks properly extended to support new map interactions.The mock additions correctly mirror the Leaflet API structure with enable/disable methods for each interaction type. The
getContainermock returns reasonable fixed dimensions.
|
1 participant
Proposed change
This PR improves the ChapterMap user experience by fully enforcing map locking and fixing layout issues on wide screens. When the map is locked, all interactions are completely disabled and only re-enabled after clicking unlock button also fixed the zoom and bounds behavior to prevent empty white space on large screens
Resolves #3012
Checklist
make check-testlocally: all warnings addressed, tests passed