Fix frequent map locking on landing page (#4301)#4307
Fix frequent map locking on landing page (#4301)#4307Akshat1931 wants to merge 4 commits intoOWASP:mainfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Summary by CodeRabbit
WalkthroughReplace unconditional section onMouseLeave deactivation with pointer-leave handling that deactivates only when the pointer truly leaves the section bounds; add section ref and containment checks. Update active-toolbar layout and adjust tests to use pointer events and assert boundary-sensitive behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ChapterMap.tsx`:
- Around line 160-176: The component currently re-locks the map automatically in
handlePointerLeave by calling setIsMapActive(false) which conflicts with the new
explicit locking behavior; remove that automatic deactivation from
handlePointerLeave (stop calling setIsMapActive(false) there) and add an
explicit lock state (e.g. const [isMapLocked, setIsMapLocked] = useState(false))
in ChapterMap; render a lock/unlock button beside the existing ShareLocation UI
(the share-location area rendered around the previous lines 309-332) that
toggles isMapLocked, and update any logic that would automatically change
isMapActive to check isMapLocked first (only allow deactivation when isMapLocked
is false). Ensure handlers reference handlePointerLeave, setIsMapActive and
isMapActive so the new lock state prevents auto-relock and provides explicit
user control.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf491a94-1d87-4ce7-aad5-795dfd3facd4
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ChapterMap.test.tsxfrontend/src/components/ChapterMap.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4307 +/- ##
==========================================
- Coverage 99.39% 99.35% -0.04%
==========================================
Files 520 520
Lines 16444 16462 +18
Branches 2294 2300 +6
==========================================
+ Hits 16344 16356 +12
Misses 62 62
- Partials 38 44 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Akshat1931
force-pushed
the
fix-map-locking-4301
branch
from
24245ab to
b98c952
Compare
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Akshat1931 thanks for opening this issue!
I’m not fully convinced a "lock map" button is the right approach. We originally added the auto-lock on mouseleave to prevent scroll conflicts—users would scroll the page, get stuck on the map, and unintentionally zoom in, making it difficult to move past. If the map stays unlocked unless users manually relock it, we could run into the same issue again.
While looking into this, I noticed a pattern: the map sometimes locks unexpectedly when the cursor moves over pin/zone areas. It seems like Leaflet overlays or popups can trigger a mouseleave-like event even when the cursor is still within the map container. Instead of adding a button, we might want to address this directly—for example, by checking whether the cursor is actually outside the map bounds before locking it.
I can see you reverted my changes by pushing your updates again. Could you please revert that?
Akshat1931
force-pushed
the
fix-map-locking-4301
branch
from
b98c952 to
76b3aa3
Compare
|
@kasya Thanks for the feedback! I’ve reverted my previous changes that introduced the lock button and aligned with your approach. I’ll now focus on fixing the pointer leave detection issue so that the map only locks when the cursor actually exits the container, avoiding unintended locks caused by Leaflet overlays or popups. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/__tests__/unit/components/ChapterMap.test.tsx">
<violation number="1" location="frontend/__tests__/unit/components/ChapterMap.test.tsx:280">
P2: The new pointer-boundary test does not control element bounds, so its "inside section" coordinates can be outside in jsdom and fail to validate the intended branch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
Updated this so the map only relocks when the pointer actually exits the map container bounds, while preserving the existing auto-lock behavior. I verified it locally, and it stays unlocked over pins/overlay areas inside the map and relocks when the pointer leaves the container. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx (2)
272-300: Consider adding an exact-edge boundary test.A case with
clientX/clientYexactly onleft/right/top/bottomwould lock in the inclusive boundary contract and prevent subtle regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ChapterMap.test.tsx` around lines 272 - 300, Add a new unit test case to ChapterMap.test.tsx that mirrors the existing "keeps map unlocked when pointer leave fires but pointer is still inside section bounds" test but uses clientX/clientY values exactly equal to one or more of the mocked DOMRect edges (left, right, top, bottom) to assert the inclusive-boundary behavior; reuse the same setup (render(<ChapterMap {...defaultProps} />), click Unlock map, mock section.getBoundingClientRect on the same section element, fireEvent.pointerLeave with clientX/clientY set to 100/300/200 as appropriate for each edge, then assert queryByText('Unlock map') is not present and that mockMap.scrollWheelZoom.disable and mockZoomControl.remove were not called to ensure the map remains unlocked on exact-edge coordinates.
231-237: Guard section lookup before dispatching pointer events.Line 231 and Line 257 use
section!; if<section>markup changes, the test fails with a low-signal runtime error. Prefer an explicit existence check, then reuse the guaranteed element.Proposed refactor
+const getSectionOrThrow = (container: HTMLElement): HTMLElement => { + const section = container.querySelector('section') + if (!section) throw new Error('Expected ChapterMap root <section> to exist') + return section as HTMLElement +} ... -const section = container.querySelector('section') -const rect = section?.getBoundingClientRect() ?? { left: 0, top: 0, right: 0, bottom: 0 } -fireEvent.pointerLeave(section!, { +const section = getSectionOrThrow(container) +const rect = section.getBoundingClientRect() +fireEvent.pointerLeave(section, { clientX: rect.left - 10, clientY: rect.top - 10, relatedTarget: null, }) ... -const section = container.querySelector('section') -const rect = section?.getBoundingClientRect() ?? { left: 0, top: 0, right: 0, bottom: 0 } -fireEvent.pointerLeave(section!, { +const section = getSectionOrThrow(container) +const rect = section.getBoundingClientRect() +fireEvent.pointerLeave(section, { clientX: rect.left - 10, clientY: rect.top - 10, relatedTarget: null, })Also applies to: 257-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/__tests__/unit/components/ChapterMap.test.tsx` around lines 231 - 237, The test is using non-null assertions on the DOM node (section!) which can cause low-signal runtime errors if the <section> markup changes; modify the test to explicitly query and guard the element before dispatching events: assign const section = container.querySelector('section'), assert its existence (e.g. if (!section) throw new Error('expected <section> in ChapterMap render') or use expect(section).not.toBeNull()), then reuse the guaranteed section variable when calling fireEvent.pointerLeave and the similar pointer event block (the lines using section! around fireEvent.pointerLeave and the other block at 257-263) so you no longer rely on the non-null assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/__tests__/unit/components/ChapterMap.test.tsx`:
- Around line 272-300: Add a new unit test case to ChapterMap.test.tsx that
mirrors the existing "keeps map unlocked when pointer leave fires but pointer is
still inside section bounds" test but uses clientX/clientY values exactly equal
to one or more of the mocked DOMRect edges (left, right, top, bottom) to assert
the inclusive-boundary behavior; reuse the same setup (render(<ChapterMap
{...defaultProps} />), click Unlock map, mock section.getBoundingClientRect on
the same section element, fireEvent.pointerLeave with clientX/clientY set to
100/300/200 as appropriate for each edge, then assert queryByText('Unlock map')
is not present and that mockMap.scrollWheelZoom.disable and
mockZoomControl.remove were not called to ensure the map remains unlocked on
exact-edge coordinates.
- Around line 231-237: The test is using non-null assertions on the DOM node
(section!) which can cause low-signal runtime errors if the <section> markup
changes; modify the test to explicitly query and guard the element before
dispatching events: assign const section = container.querySelector('section'),
assert its existence (e.g. if (!section) throw new Error('expected <section> in
ChapterMap render') or use expect(section).not.toBeNull()), then reuse the
guaranteed section variable when calling fireEvent.pointerLeave and the similar
pointer event block (the lines using section! around fireEvent.pointerLeave and
the other block at 257-263) so you no longer rely on the non-null assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87ac9cfc-fbe0-4f4f-a105-170b44ef2a92
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
|
@Akshat1931 @kasya Thanks guys! |
3 participants
Proposed change
Resolves #4301
This PR fixes the frequent map locking issue on the landing page by removing the automatic lock-on-mouseleave behavior and adding an explicit lock button.
Why this change is needed:
Poor UX: The map re-locks every time the cursor leaves the map container, making it impossible to pan/zoom smoothly.
Accessibility: Users with motor impairments or slow movements get frustrated by constant re-locking.
Technical Implementation:
frontend/src/components/ChapterMap.tsx: Removed onMouseLeave handler that auto-locked the map. Added a lock button (FaLock icon) next to the location sharing button, visible when the map is unlocked.
Checklist
make check-testlocally: all warnings addressed, tests passed