fix: stop mapbox pins from flickering

[mkouzel-yext]

Fixes a bug in the MapboxMap component where all Mapbox pins are removed and re-created when any pin is clicked. This bug was introduced when selectedResult was added to the dependency array of the effect hook that runs when all results change (i.e. a search is made) - by design, this hook destroys all current pins and makes new ones so it should not be re-run when only selectedResult changes.

This change:

• Extracts marker creation into it's own helper function, createMarker
• Adds an effect hook to update pins rendered with renderPin instead of PinComponent when they are selected. PinComponent already has an effect hook that handles selection, but renderPin does not.
• Adds comments to each MapboxMap effect hook to give context for what they do (helpful for me, personally)

J=WAT-5390

[github-actions]

Current unit coverage is 89.10973084886129%
Current visual coverage is 69.6822832525579%
Current combined coverage is 92.16402760860738%

[coveralls]

coverage: 84.693% (-0.5%) from 85.188%
when pulling 4ce7434 on prevent-pin-flickering
into 3af4696 on main.

[Copilot]

Pull request overview

This PR fixes a bug where all Mapbox map pins were being removed and recreated whenever any pin was clicked, causing flickering. The issue was caused by selectedResult being in the dependency array of the effect that manages all markers.

Changes:

• Extracted marker creation logic into a reusable createMarker helper function
• Removed selectedResult from the main marker creation effect's dependency array to prevent unnecessary recreation of all markers
• Added an effect to handle selection changes for markerOptionsOverride by recreating only affected markers
• Added/modified an effect to handle selection changes for PinComponent by re-rendering components
• Enhanced MapPin component to scale up when selected using the selected prop

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/components/MapboxMap.tsx	Core fix: refactored marker creation, removed selectedResult from main effect dependencies, added selection handling effects, added descriptive comments to effects
test-site/src/components/MapPin.tsx	Added dynamic sizing based on selection state using useMemo
test-site/src/pages/LocationsPage.tsx	Commented out markerOptionsOverride prop (test/development code)

Comments suppressed due to low confidence (4)

src/components/MapboxMap.tsx:482

• The event listener cleanup creates a new arrow function which won't match the one added in createMarker (line 271), so removeEventListener won't work. Event listeners added with arrow functions need to be removed with a reference to the exact same function. This could lead to memory leaks. Consider either: (1) storing the event handler function reference with each marker, or (2) using marker.remove() which should handle cleanup, or (3) ensuring the cleanup happens elsewhere.

          markers.current.forEach((marker, i) => {
            marker?.getElement().removeEventListener('click', () => handlePinClick(locationResults[i]));
          });

src/components/MapboxMap.tsx:569

• When both markerOptionsOverride and PinComponent are provided, changing selectedResult triggers both effects (lines 506-547 and 549-569). The first effect recreates affected markers with the correct selected state, then the second effect re-renders all PinComponents unnecessarily. Consider making the second effect skip markers that were just recreated by the first effect, or preventing the second effect from running when the first effect has already handled the update.

  // re-render custom PinComponent on selection changes to update the visual state
  useEffect(() => {
    const mapbox = map.current;
    if (!mapbox || !PinComponent) {
      return;
    }
    markerData.current.forEach(({ marker, result, index }) => {
      const element = marker?.getElement?.();
      if (!element) {
        return;
      }
      attachPinComponent(element, (
        <PinComponent
          index={index}
          mapbox={mapbox}
          result={result}
          selected={selectedResult === result}
        />
      ));
    });
  }, [attachPinComponent, PinComponent, selectedResult]);

src/components/MapboxMap.tsx:569

• This effect re-renders ALL PinComponents whenever selectedResult changes, which is inefficient. Similar to the markerOptionsOverride effect (lines 506-547), this should only re-render the two affected pins: the previously selected pin (to unselect it) and the newly selected pin (to select it). Consider updating only the pins that actually changed their selection state by tracking the previous selectedResult and updating only those two markers.

  // re-render custom PinComponent on selection changes to update the visual state
  useEffect(() => {
    const mapbox = map.current;
    if (!mapbox || !PinComponent) {
      return;
    }
    markerData.current.forEach(({ marker, result, index }) => {
      const element = marker?.getElement?.();
      if (!element) {
        return;
      }
      attachPinComponent(element, (
        <PinComponent
          index={index}
          mapbox={mapbox}
          result={result}
          selected={selectedResult === result}
        />
      ));
    });
  }, [attachPinComponent, PinComponent, selectedResult]);

src/components/MapboxMap.tsx:482

• The removeEventListener call creates a new arrow function, which won't match the one added in createMarker (line 271), making this call ineffective. Since marker.remove() is called immediately after in removeMarkers() (line 483), which should clean up event listeners, this line can be removed as it serves no purpose.

          markers.current.forEach((marker, i) => {
            marker?.getElement().removeEventListener('click', () => handlePinClick(locationResults[i]));
          });

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When locationResults changes, the selectedResult effects (lines 506-547 and 549-569) may run with stale markerData.current before the new markers are created. Consider resetting previousSelectedResult.current to undefined in the locationResults effect cleanup function, or at the start of the locationResults effect, to prevent attempting to update markers that no longer exist.

Suggested change

	useEffect(() => {
	useEffect(() => {
	// Reset previousSelectedResult to avoid referencing markers that have been removed.
	previousSelectedResult.current = undefined;

[Copilot]

The renderPin function call is missing the selected parameter. According to the PinComponentProps type definition (line 38), the selected property should be passed to renderPin. This should be: renderPin({ index, mapbox, result, selected, container: el })

Suggested change

	renderPin({ index, mapbox, result, container: el });
	renderPin({ index, mapbox, result, selected, container: el });

[Copilot]

The optional chaining marker?.getElement() will return undefined if marker is falsy, and then .addEventListener() will throw an error. Either remove the optional chaining on marker (since marker should always be defined here), or use the pattern marker.getElement()?.addEventListener() or const element = marker?.getElement?.(); if (element) { element.addEventListener(...) }

Suggested change

	marker?.getElement().addEventListener('click', () => handlePinClick(result));
	marker.getElement()?.addEventListener('click', () => handlePinClick(result));

[Copilot]

This commented-out code should be removed before merging. If it's needed for future reference or testing, consider documenting why it's being kept or move it to a separate branch/documentation.