Fix ChapterMap locked state with explicit unlock #3130
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughChapterMap now initializes with interactions disabled and shows an explicit unlock overlay/button. Introduces setMapInteractivity and unlockMap, uses an Unlock button to enable interactions, adds Escape-to-relock handling, removes mouseout/click auto-toggle, and updates tests to cover interactivity, overlay, ARIA, zoom-control visibility, clustering, and cleanup. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
483-535: Consider adding cleanup test for the Escape key listener.While the Escape key handler includes proper cleanup (line 203 in ChapterMap.tsx), consider adding a test to verify that the event listener is removed when the component unmounts. This would provide additional confidence in the cleanup behavior.
Example test for cleanup verification
it('cleans up Escape key listener on unmount', () => { const addEventListenerSpy = jest.spyOn(window, 'addEventListener') const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener') const { unmount } = render(<ChapterMap {...defaultProps} />) const handleEscape = addEventListenerSpy.mock.calls[0]?.[1] unmount() expect(removeEventListenerSpy).toHaveBeenCalledWith('keydown', handleEscape) addEventListenerSpy.mockRestore() removeEventListenerSpy.mockRestore() })
📜 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 (3)
📚 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
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (11)
frontend/src/components/ChapterMap.tsx (5)
36-44: LGTM! Clean interactivity control implementation.The function correctly toggles all major Leaflet interaction handlers. The approach is clean and centralized.
46-50: LGTM! Proper guard and state management.The early return guard and sequential state update are correct.
75-75: LGTM! Correctly enforces locked-by-default behavior.This aligns with the PR objective to prevent accidental interactions on load.
195-204: LGTM! Proper global event listener with cleanup.The Escape key handler is correctly implemented with proper cleanup and appropriate dependency tracking.
232-242: LGTM! Proper pointer-events pattern and accessibility.The overlay implementation correctly uses
pointer-events-noneon the wrapper withpointer-events-autoon the button, and includes proper accessibility attributes.frontend/__tests__/unit/components/ChapterMap.test.tsx (6)
14-37: LGTM! Comprehensive interaction handler mocks.All new Leaflet interaction handlers are properly mocked with enable/disable methods.
183-197: LGTM! Comprehensive initialization coverage.Tests correctly verify that all interaction handlers are disabled on init and that the marker cluster group is properly created and added.
310-339: LGTM! Thorough unlock overlay testing.Tests correctly verify overlay removal, interaction handler enabling, and accessibility attributes.
483-535: Verify the window event testing pattern.The Escape key tests have good coverage, but the pattern
fireEvent.keyDown(window, { key: 'Escape' })may not work as expected sincefireEventis designed for DOM elements, not the window object.According to React Testing Library best practices, window events should be tested using native event dispatch:
const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }) window.dispatchEvent(event)Please verify that these tests are actually triggering the event handler as intended.
🔎 Recommended pattern for testing window events
- fireEvent.keyDown(window, { key: 'Escape' }) + const escapeEvent = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }) + window.dispatchEvent(escapeEvent)Apply this pattern to lines 496, 510, and 530.
438-446: LGTM! Correct zoom control visibility testing.Test properly verifies that zoom control is added when the map is unlocked.
537-552: LGTM! Proper pointer-events structure validation.Tests correctly verify the CSS classes for the pointer-events pattern on both the overlay wrapper and unlock button.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (6)
frontend/__tests__/unit/components/ChapterMap.test.tsx (6)
14-37: LGTM! Comprehensive mock interaction handlers.The mock structure correctly covers all map interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) with enable/disable methods, aligning with the Leaflet API and the PR's explicit lock/unlock requirements.
183-197: LGTM! Proper initialization validation.These tests correctly verify that:
- All interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) are disabled on initial render—the core requirement for the locked state.
- The marker cluster group is created and added to the map.
310-339: LGTM! Thorough unlock flow validation.These tests comprehensively verify the unlock behavior:
- Overlay is removed when the unlock button is clicked.
- All six interaction handlers are enabled.
- Proper accessibility attributes (
type="button",aria-label="Unlock map") are present.The use of
.closest('button')!with non-null assertion is acceptable here since the test would fail earlier if the button doesn't exist.
438-446: LGTM! Zoom control visibility correctly validated.The test properly verifies that the zoom control is added to the map only after the unlock button is clicked, maintaining a clean UI in the locked state.
536-553: LGTM! Proper event listener cleanup verification.This test correctly validates that the Escape key listener is removed on unmount, preventing memory leaks. The spy-based approach is appropriate for verifying cleanup behavior.
The pattern of finding the handler via
addEventListenerSpy.mock.calls.find(([event]) => event === 'keydown')works well in this isolated test context and properly restores the spies afterward.
556-571: LGTM! Critical pointer-events structure validation.These tests ensure the correct CSS pointer-events configuration:
- The overlay wrapper has
pointer-events-none(line 560) so it doesn't block map interactions when unlocked.- The unlock button has
pointer-events-auto(line 569) so it remains clickable despite the parent'spointer-events-none.This is essential for the UX: the button must be accessible in the locked state, but the overlay shouldn't interfere with map interactions once unlocked.
|
@kasya can u review the pr ? |
There was a problem hiding this comment.
@kris70lesgo I sure can review this but first you're required to:
- address any comments from CodeRabbit ☑️
- ensure there are no new issues from Sonar (there are currently 10 new issues introduced in this PR)
- Run
make check-testlocally and address all found issues (which you didn't, though you added a checkmark next to it)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ChapterMap.tsx (1)
14-14: Remove duplicate import.Line 10 already imports
'leaflet.markercluster', making this import redundant.🔎 Proposed fix
-import 'leaflet.markercluster'
📜 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
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/ChapterMap.test.tsx
🔇 Additional comments (5)
frontend/src/components/ChapterMap.tsx (5)
36-44: LGTM! Well-centralized interaction control.The function cleanly toggles all six Leaflet interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) using a unified enable/disable pattern.
46-50: LGTM! Clear unlock logic.The function properly guards against null references and correctly enables both map interactions and the active state.
232-242: Verify that the overlay z-index blocks marker and cluster interactions.The overlay uses
z-[500]to block map interactions, and the pointer-events strategy (none on wrapper, auto on button) is correct. However, please verify that marker clicks and cluster interactions are fully blocked when the map is locked, as Leaflet markers can have their own z-index values.Test manually by:
- Loading the map in locked state
- Attempting to click on markers and clusters through the overlay
- Verifying no popups open and no interactions occur
75-75: LGTM! Enforces initial locked state.Calling
setMapInteractivity(mapRef.current, false)after map initialization correctly implements the requirement to start with all interactions disabled.
1-273: No action needed. Leaflet 1.9.4 has no known React 19 compatibility issues—Leaflet is library-agnostic and the React compatibility concerns you may have encountered relate to react-leaflet (not used here), which requires v5 for React 19. The code correctly uses vanilla Leaflet with proper ref handling and useEffect cleanup, which is safe for React 19.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ChapterMap.tsx (1)
35-43: Consider extractingsetMapInteractivityoutside the component.Since this function doesn't depend on any component state or props (it only uses the
mapparameter), it can be moved outside the component to avoid recreation on each render.🔎 Proposed refactor
+const setMapInteractivity = (map: L.Map, enabled: boolean) => { + const action = enabled ? 'enable' : 'disable' + map.dragging[action]() + map.touchZoom[action]() + map.doubleClickZoom[action]() + map.scrollWheelZoom[action]() + map.boxZoom[action]() + map.keyboard[action]() +} + const ChapterMap = ({ geoLocData, showLocal, style, userLocation, onShareLocation, }: { ... }) => { ... - const setMapInteractivity = (map: L.Map, enabled: boolean) => { - const action = enabled ? 'enable' : 'disable' - map.dragging[action]() - map.touchZoom[action]() - map.doubleClickZoom[action]() - map.scrollWheelZoom[action]() - map.boxZoom[action]() - map.keyboard[action]() - }
📜 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 (3)
📚 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
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (9)
frontend/src/components/ChapterMap.tsx (3)
45-49: LGTM!The
unlockMapfunction has proper null checking and cleanly centralizes the unlock logic.
194-208: Good implementation addressing the Escape key race condition.The handler now properly calls
stopPropagation()andpreventDefault()to prevent conflicts with other global Escape handlers (like MultiSearch). The early return guard for!isMapActiveensures the handler only acts when the map is unlocked.
236-246: Well-structured accessible unlock overlay.Good use of
pointer-events-noneon the overlay container withpointer-events-autoon the button. The accessibility attributes (type="button",aria-label,focus-visible:ring-2) ensure keyboard users can interact with the unlock flow.frontend/__tests__/unit/components/ChapterMap.test.tsx (6)
14-37: LGTM!Comprehensive mock setup for all map interaction handlers, consistent with the component's
setMapInteractivityimplementation.
183-197: LGTM!Good test coverage verifying that all interaction handlers are disabled on initialization, aligning with the PR objective of a fully locked initial state.
320-333: LGTM!Thorough test coverage for the unlock flow, verifying all six interaction handlers are enabled when the user clicks the unlock button.
489-516: Good implementation addressing the past review concern.The tests now correctly use
globalThis.dispatchEvent(new KeyboardEvent('keydown', ...))instead offireEvent.keyDown(window, ...), ensuring the window-level event listener is properly triggered. The additional assertions forpreventDefaultandstopPropagation(lines 514-515) verify that the race condition mitigation is working.
550-567: LGTM!Proper verification of event listener cleanup on unmount. The spy approach correctly captures the registered handler and confirms it's removed during cleanup.
570-584: LGTM!Good structural tests verifying the pointer-events CSS pattern that enables the unlock button to be clickable while the rest of the overlay passes events through to the map.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ChapterMap.test.tsx (3)
494-495: Use consistent button selection pattern.Line 495 clicks the result of
getByText('Unlock map')directly, while other tests (lines 313, 323, 338, 444, 471, 481) usegetByText('Unlock map').closest('button')with null checks. Theclosest('button')pattern is more defensive and ensures you're interacting with the button element even if the text is in a child element.🔎 Suggested consistent approach
- const unlockButton = getByText('Unlock map') - fireEvent.click(unlockButton) + const unlockButton = getByText('Unlock map').closest('button') + expect(unlockButton).not.toBeNull() + fireEvent.click(unlockButton as HTMLElement)
503-515: Consider removing implementation detail assertions.The test verifies that
preventDefaultandstopPropagationwere called on the keyboard event. This tests implementation details rather than behavior. The re-lock functionality is already validated by checking the UI state (line 509) and mock calls (lines 510-511).🔎 Simplified test approach
The assertions at lines 503-504 and 514-515 can be removed while still fully testing the re-lock behavior:
- const event = new KeyboardEvent('keydown', { key: 'Escape', cancelable: true }) - const preventDefaultSpy = jest.spyOn(event, 'preventDefault') - const stopPropagationSpy = jest.spyOn(event, 'stopPropagation') - - globalThis.dispatchEvent(event) + globalThis.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })) // Verify map is locked again expect(getByText('Unlock map')).toBeInTheDocument() expect(mockMap.dragging.disable).toHaveBeenCalled() expect(mockMap.scrollWheelZoom.disable).toHaveBeenCalled() - - // Verify event was contained to prevent race conditions - expect(preventDefaultSpy).toHaveBeenCalled() - expect(stopPropagationSpy).toHaveBeenCalled()
579-584: Use consistent button selection pattern.Line 582 uses
getByText('Unlock map')withoutclosest('button'), inconsistent with the safer pattern used throughout the file (lines 313, 323, 338, 444, 471, 481). If the button text is rendered in a child element (e.g., a<span>), this test would incorrectly check the child's class instead of the button's class.🔎 Suggested consistent approach
- const button = getByText('Unlock map') + const button = getByText('Unlock map').closest('button') + expect(button).not.toBeNull() expect(button).toHaveClass('pointer-events-auto')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (6)
frontend/__tests__/unit/components/ChapterMap.test.tsx (6)
14-37: LGTM!The mock interaction handlers are comprehensive and correctly structured to test the full lock/unlock lifecycle for all map interactions.
183-197: LGTM!The tests correctly verify that all interaction handlers are disabled on initialization, ensuring the map starts in a fully locked state as required by the PR objectives.
310-342: LGTM!The unlock button tests are comprehensive, covering interaction enabling, overlay removal, and accessibility attributes. The defensive pattern with
closest('button')and null checks is good practice.
502-506: LGTM!The Escape key tests correctly use
globalThis.dispatchEvent(new KeyboardEvent(...))to trigger window-level keyboard events, properly testing the re-lock behavior.
550-567: LGTM!The cleanup test properly verifies that the Escape key listener is removed on unmount, preventing memory leaks.
Note: The test finds the handler by searching for the 'keydown' event listener in
addEventListenercalls (lines 557-559). This works for the current implementation but assumes only one keydown handler exists. Given the component's scope, this is acceptable.
571-577: LGTM!Testing for the
pointer-events-noneclass is appropriate here since pointer events are critical to the overlay functionality—allowing clicks through to the unlock button while blocking other interactions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
530-531: Consider using the consistent button selection pattern.Line 530-531 directly clicks the result of
getByText('Unlock map'), while most other tests in this file use the more explicit pattern:const unlockButton = getByText('Unlock map').closest('button') expect(unlockButton).not.toBeNull() fireEvent.click(unlockButton as HTMLElement)While the current approach works due to event bubbling, using
closest('button')is more explicit and maintains consistency with lines 313-315, 323-325, 338-341, 444-446, 471-473, and 481-483.Suggested refactor for consistency
- const unlockButton = getByText('Unlock map') - fireEvent.click(unlockButton) + const unlockButton = getByText('Unlock map').closest('button') + expect(unlockButton).not.toBeNull() + fireEvent.click(unlockButton as HTMLElement)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (8)
frontend/__tests__/unit/components/ChapterMap.test.tsx (8)
14-37: LGTM: Mock interaction handlers are well-structured.The added mock handlers (dragging, touchZoom, doubleClickZoom, boxZoom, keyboard) correctly mirror the Leaflet API structure and support comprehensive testing of the new interaction lock/unlock behavior.
183-197: LGTM: Initialization tests properly verify locked state.The test correctly asserts that all interaction handlers are disabled on initialization, aligning with the PR objective of enforcing a fully locked map state on load. Good separation of cluster group creation test.
310-342: LGTM: Unlock button tests are comprehensive.The tests thoroughly verify:
- Overlay removal on unlock
- All six interaction handlers enabled (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard)
- Proper accessibility attributes (type, aria-label)
The button selection pattern using
closest('button')with null checks is appropriate for these tests.
441-450: LGTM: Zoom control visibility test is correct.The test properly verifies that the zoom control is created and added to the map when the unlock button is clicked.
456-486: LGTM: Share location button visibility tests are thorough.The tests comprehensively cover the three visibility scenarios:
- Not shown when map is locked
- Shown after unlocking
- Not rendered when callback prop is missing
489-541: LGTM: Escape key re-lock tests are comprehensive and correctly implemented.The tests properly verify:
- Map re-locks when Escape is pressed (interactions disabled, overlay shown)
- No-op behavior when Escape pressed while already locked
- Zoom control removed on re-lock
The use of
globalThis.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }))correctly addresses the previous review concern about triggering window-level keyboard events.
543-560: LGTM: Event listener cleanup test is well-designed.The test properly verifies that the Escape key listener is cleaned up on component unmount by:
- Spying on addEventListener/removeEventListener
- Finding the registered 'keydown' handler
- Confirming the same handler is removed
- Cleaning up spies
563-579: LGTM: Pointer events structure tests ensure correct CSS layering.The tests properly verify the pointer-events CSS classes:
- Overlay wrapper has
pointer-events-noneto avoid blocking underlying elements- Unlock button has
pointer-events-autoto remain clickableThis ensures the overlay behaves correctly without interfering with other UI interactions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
444-486: LGTM! Zoom control and share location visibility tests are solid.The tests correctly verify:
- Zoom control appears only after unlocking
- Share location button visibility is gated by map active state and prop presence
Minor note: Lines 471 and 481 name the variable
overlaywhen it's actually the button element. Consider renaming tounlockButtonfor clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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
📚 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 (5)
frontend/__tests__/unit/components/ChapterMap.test.tsx (5)
14-37: LGTM! Comprehensive interaction control mocks.The mock structure correctly covers all Leaflet interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) with enable/disable methods, enabling thorough testing of the explicit unlock feature.
183-197: LGTM! Initialization tests validate locked state.These tests correctly verify that:
- All interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) are disabled on initial load
- Marker cluster group is properly created and added to the map
This aligns with the PR objective to enforce a fully locked ChapterMap state on initial load.
310-342: LGTM! Unlock button tests are comprehensive.The tests properly validate the unlock flow:
- Overlay removal on button click
- All interaction handlers (dragging, touchZoom, doubleClickZoom, scrollWheelZoom, boxZoom, keyboard) enabled
- Proper ARIA attributes (type, aria-label) for accessibility
The defensive null checks and consistent button selection pattern make the tests clear and maintainable.
489-562: LGTM! Escape key re-lock tests are thorough and correctly implemented.These tests properly validate the Escape key behavior:
- Re-locks the map (disables interactions, shows overlay)
- No-op when map is already locked
- Removes zoom control on re-lock
- Cleans up event listener on unmount
The implementation correctly uses
globalThis.dispatchEvent(new KeyboardEvent(...))to trigger window-level keydown events, addressing the past review feedback. The event listener cleanup test is particularly good for preventing memory leaks.
564-580: LGTM! Pointer events structure tests validate critical overlay behavior.These tests verify the CSS class structure that enables the overlay to function correctly:
- Overlay wrapper has
pointer-events-noneto pass clicks through- Unlock button has
pointer-events-autoto receive clicksWhile testing CSS classes is generally an implementation detail, these are structural requirements that make the explicit unlock feature work properly, so this coverage is valuable.
|
@kasya, thanks for the review. |
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Hi @kris70lesgo! Found a couple of issues you'll need to address with this PR ⬇️
There was a problem hiding this comment.
Previously the map would lock back on loosing focus. After your changes I'm only able to lock the map back with Escape button but I believe we need to keep that lock on loosing focus as well. 🤔
There was a problem hiding this comment.
Also I'm still able to click into pins when map is closed:
2 participants
Proposed change
Resolves #3012
This pr fixes the ChapterMap locked/unlocked interaction behavior by enforcing a fully locked map state on initial load and enabling map interactions only through an explicit user action.
Summary of changes
This approach prevents accidental map interactions while scrolling and aligns the behavior with explicit user intent.
Checklist
make check-testlocally and all tests passed