Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMap controls and share-location UI are hidden by default and only enabled after the user unlocks/activates the map. Tests and a Leaflet mock were updated to support creating/removing a custom zoom control; the component manages a zoomControlRef and conditionally renders location-sharing UI based on isMapActive. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
412-436: Good test coverage for zoom control visibility.The tests properly verify that the zoom control is hidden initially and appears when the map is unlocked.
Consider adding a test to verify cleanup when the component unmounts while the map is active:
it('removes zoom control when component unmounts while active', () => { const { getByText, unmount } = render(<ChapterMap {...defaultProps} />) const overlay = getByText('Unlock map').closest('button') fireEvent.click(overlay) expect(mockZoomControl.addTo).toHaveBeenCalledWith(mockMap) unmount() expect(mockZoomControl.remove).toHaveBeenCalled() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx(4 hunks)frontend/src/components/ChapterMap.tsx(4 hunks)
🧰 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
🧬 Code graph analysis (1)
frontend/src/components/ChapterMap.tsx (1)
frontend/src/types/button.ts (1)
Button(4-9)
🔇 Additional comments (6)
frontend/src/components/ChapterMap.tsx (3)
32-32: LGTM!The
zoomControlRefis properly typed and follows the same pattern as other refs in the component.
46-46: LGTM!Disabling the default zoom control is correct. The custom zoom control is properly managed via the new
useEffecthook based on map interaction state.
241-266: LGTM!The share location button is now correctly hidden by default and only appears when the map is active. The conditional rendering logic properly checks both
isMapActiveandonShareLocation, ensuring the button displays only when appropriate.frontend/__tests__/unit/components/ChapterMap.test.tsx (3)
41-57: LGTM!The zoom control mock is properly structured with
addToandremovemethods, correctly simulating the Leaflet zoom control API for testing.
146-146: LGTM!The test correctly verifies that the map is initialized with
zoomControl: false, matching the implementation change.
438-470: LGTM!The test suite comprehensively verifies share location button visibility across all scenarios: hidden by default, displayed when the map is unlocked, and absent when no callback is provided.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/e2e/pages/ChapterDetails.spec.ts (1)
38-39: LGTM! Test correctly validates the unlock flow.The test properly clicks the "Unlock map" button before asserting control visibility, aligning with the PR objective to hide controls until explicit user interaction.
Optional enhancement: Consider asserting that controls are NOT visible before unlocking to make the test more robust:
await expect(page.locator('#chapter-map').locator('img').nth(1)).toBeVisible() +await expect(page.getByRole('button', { name: 'Zoom in' })).not.toBeVisible() +await expect(page.getByRole('button', { name: 'Zoom out' })).not.toBeVisible() +await expect(page.getByRole('button', { name: 'Marker' })).not.toBeVisible() + await page.getByRole('button', { name: 'Unlock map' }).click()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/e2e/pages/ChapterDetails.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/e2e/pages/ChapterDetails.spec.ts
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Shofikul-Isl4m works great 👍🏼
Thanks!
* hide map controls by default * update e2e test * add cleanup func and solve sonarcloud issue * run make check-test
2 participants
Resolves #2896
This PR hides map controls (zoom buttons and share location button) by default and only displays them after the user explicitly clicks the "Unlock map" button.
Checklist
make check-testlocally and all tests passed