Skip to content

Clear POI overlay markers when all categories are unchecked - #1978

Merged
alisa911 merged 3 commits into
mainfrom
fix-poi-overlay-not-cleared
Sep 7, 2026
Merged

Clear POI overlay markers when all categories are unchecked#1978
alisa911 merged 3 commits into
mainfrom
fix-poi-overlay-not-cleared

Conversation

@vshcherb

@vshcherb vshcherb commented Sep 3, 2026

Copy link
Copy Markdown
Member

Fixes #1945 — after the POI Overlay is disabled in Configure Map, the orange POI icons stayed on the map until a manual page refresh.

Cause

PoiLayer's main effect started with:

if ((!isEmpty(ctx.showPoiCategories) && zoom !== prevZoom) || move || isTypeChange) { ... }
else if (isEmpty(ctx.showPoiCategories)) { /* clearPoiList() lives here */ }

move is the moveend timestamp from useZoomMoveMapHandlers and is never reset, so it stays truthy after the first map move (which happens on load). The first branch therefore always won; inside it if (ctx.showPoiCategories.length > 0) was false, so nothing happened — and the branch that calls clearPoiList() when the category list becomes empty was never reached.

Fix

Check the empty-categories case first, then the zoom/move/type-change refresh. The nested ctx.showPoiCategories.length > 0 check is now redundant and is dropped.

Test

New selenium test search/100-poi-overlay-disable.mjs: enables the Cafe and restaurant category, checks the markers appear, unchecks it, and waits for them to disappear. It fails on main (waitByRemoved times out on se-poi-marker-background-#f8931d-circle) and passes with the fix.

Verified locally against localhost:3000 (dev server proxied to test.osmand.net): search (11), map (2) and menu (6) suites all pass.

AI disclaimer

Produced with Claude Code (Opus 5).

Prompts used (summarised):

  1. Reproduce and fix issue POI Overlay icons remain visible on the map after the overlay is disabled #1945, using the local web dev server (no postgres) and the local maps folder.
  2. Create the pull request, and add a web UI test for this — it is a bad enough problem to be worth covering.

Decided by the agent, not requested explicitly:

  • Root-cause choice of fix: reordering the branches in PoiLayer's effect (checking the empty-category case first) rather than resetting the move flag after each refresh; the redundant nested length > 0 check was dropped as part of it.
  • Placement, name and shape of the regression test (tests/selenium/src/tests/search/100-poi-overlay-disable.mjs), modelled on the existing 91-poi-markers-test.mjs.
  • Running the neighbouring search, map and menu suites as a regression check, and confirming the new test fails on the unfixed code before claiming it covers the bug.

The POI layer effect tested `... || move || isTypeChange` before the
empty-categories branch. `move` is a moveend timestamp that stays truthy
after the first map move, so the first branch always won, and the branch
that removes the layer when the category list became empty was never
reached — the orange POI markers stayed on the map until a page reload.

Check for empty categories first, then the zoom/move/type-change refresh.

Add selenium test search/100-poi-overlay-disable.mjs, which enables a POI
category, then unchecks it and waits for the markers to disappear. It fails
on the old code (waitByRemoved times out) and passes with the fix.

Fixes #1945

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@alisa911 alisa911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test needs to be reworked, it duplicates the existing test and breaks the test order and test conventions. The empty-categories branch in PoiLayer also needs rework

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The asynchronous POI update race must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes POI markers remaining visible after all categories are disabled.

Changes:

  • Prioritizes clearing markers when no POI categories are selected.
  • Adds Selenium regression coverage for disabling the overlay.
File summaries
File Review
map/src/map/layers/PoiLayer.js Reorders empty-category handling. Moderate (2 votes): pending asynchronous layer creation can restore cleared markers; invalidate and re-check pending requests before applying results.
tests/selenium/src/tests/search/91-poi-markers-test.mjs Verifies markers disappear after disabling the category.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread map/src/map/layers/PoiLayer.js
@alisa911
alisa911 merged commit 9b99774 into main Sep 7, 2026
@alisa911
alisa911 deleted the fix-poi-overlay-not-cleared branch September 7, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

POI Overlay icons remain visible on the map after the overlay is disabled

3 participants