From c0e6c3c0ba5d3a91c3f511062ae477ea2035a6c8 Mon Sep 17 00:00:00 2001 From: Jacques Ikot Date: Wed, 5 Mar 2025 15:25:52 +0100 Subject: [PATCH] fix: map widget marker onClick duplication (#39517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description When clicking on a marker in the Map Widget, the onMarkerClick event was being triggered twice for a single click, causing duplicate actions and potentially unexpected behavior in applications. ```js // In MapWidget/widget/index.tsx onMarkerClick = (lat?: number, long?: number, title?: string) => { this.disableDrag(true); const selectedMarker = { lat: lat, long: long, title: title, }; console.log("🚀 ~ MapWidget ~ title:", title); // This was logging twice this.props.updateWidgetMetaProperty("selectedMarker", selectedMarker, { triggerPropertyName: "onMarkerClick", dynamicString: this.props.onMarkerClick, event: { type: EventType.ON_MARKER_CLICK, }, }); }; ``` ## Root Cause This happens because: 1. The Marker component was adding duplicate event listeners to the Google Maps marker. 2. Two separate useEffect hooks were both adding "click" event listeners: - One during marker initialisation - Another in a separate useEffect that tracks the onClick prop ```js // First listener in initialization useEffect googleMapMarker.addListener("click", () => { if (onClick) onClick(); }); // Second listener in a separate useEffect marker.addListener("click", () => { if (onClick) onClick(); }); ``` 3. When a marker was clicked, both event listeners fired, causing the onClick callback to execute twice. 4. This resulted in onMarkerClick being called twice for a single user interaction. ## Solution Modify the `Marker.tsx` component to avoid adding duplicate event listeners: 1. Remove the click event listener from the initialisation useEffect 2. Add proper cleanup for event listeners using clearListeners and removeListener 3. Store the listener reference to properly remove it in the cleanup function 4. Apply the same pattern to the dragend event listener ```js // track on onclick - with proper cleanup useEffect(() => { if (!marker) return; // Clear existing click listeners before adding a new one google.maps.event.clearListeners(marker, "click"); // Add the new click listener const clickListener = marker.addListener("click", () => { if (onClick) onClick(); }); // Return cleanup function return () => { google.maps.event.removeListener(clickListener); }; }, [marker, onClick]); ``` ## Why this works 1. Properly cleans up existing listeners before adding new ones 2. Ensures only one click handler is active at any time 3. Prevents event handler duplication during component re-renders ## Testing Added a Cypress test that verifies the marker click event triggers exactly once per click: 1. Creates a map widget with a marker 2. Uses a JS Object to track click count 3. Verifies the click count increments by exactly 1 for each click 4. Ensures no duplicate events are triggered Fixes #39514 ## Automation /ok-to-test tags="@tag.Widget, @tag.Sanity, @tag.Maps, @tag.Binding" ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 3c025ebd9dd85a3c409bc1582e13e324d8dbb2cd > Cypress dashboard. > Tags: `@tag.Widget, @tag.Sanity, @tag.Maps, @tag.Binding` > Spec: >
Wed, 05 Mar 2025 11:49:24 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Bug Fixes** - Enhanced the behavior of map markers so they respond more reliably to user interactions like clicking and dragging, addressing issues that could affect overall stability. - **Tests** - Added comprehensive tests to ensure marker functionality remains consistent and dependable during use. --- .../Widgets/{Others => Map}/MapWidget_Spec.ts | 0 .../{Others => Map}/MapWidget_loading_Spec.ts | 0 .../MapWidget/component/MapComponent.test.tsx | 121 ++++++++++++++++++ .../widgets/MapWidget/component/Marker.tsx | 29 +++-- 4 files changed, 140 insertions(+), 10 deletions(-) rename app/client/cypress/e2e/Regression/ClientSide/Widgets/{Others => Map}/MapWidget_Spec.ts (100%) rename app/client/cypress/e2e/Regression/ClientSide/Widgets/{Others => Map}/MapWidget_loading_Spec.ts (100%) create mode 100644 app/client/src/widgets/MapWidget/component/MapComponent.test.tsx diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Others/MapWidget_Spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Map/MapWidget_Spec.ts similarity index 100% rename from app/client/cypress/e2e/Regression/ClientSide/Widgets/Others/MapWidget_Spec.ts rename to app/client/cypress/e2e/Regression/ClientSide/Widgets/Map/MapWidget_Spec.ts diff --git a/app/client/cypress/e2e/Regression/ClientSide/Widgets/Others/MapWidget_loading_Spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Widgets/Map/MapWidget_loading_Spec.ts similarity index 100% rename from app/client/cypress/e2e/Regression/ClientSide/Widgets/Others/MapWidget_loading_Spec.ts rename to app/client/cypress/e2e/Regression/ClientSide/Widgets/Map/MapWidget_loading_Spec.ts diff --git a/app/client/src/widgets/MapWidget/component/MapComponent.test.tsx b/app/client/src/widgets/MapWidget/component/MapComponent.test.tsx new file mode 100644 index 0000000000..998a4d382d --- /dev/null +++ b/app/client/src/widgets/MapWidget/component/MapComponent.test.tsx @@ -0,0 +1,121 @@ +import React from "react"; +import { render } from "@testing-library/react"; +import Marker from "../component/Marker"; + +// Mock the google maps API +const mockAddListener = jest.fn().mockImplementation((event, callback) => { + // Store the callback to simulate click events + if (event === "click") { + ( + mockAddListener as unknown as { + clickCallback: (...args: unknown[]) => void; + } + ).clickCallback = callback; + } + + return "listener-id"; // Return a mock listener ID +}); + +const mockRemoveListener = jest.fn(); +const mockSetMap = jest.fn(); +const mockSetIcon = jest.fn(); +const mockSetPosition = jest.fn(); +const mockSetTitle = jest.fn(); + +// Add type declaration for the global google object +declare global { + interface Window { + google: unknown; + } +} + +// Mock the google object +(global as unknown as { google: unknown }).google = { + maps: { + Marker: jest.fn().mockImplementation(() => ({ + setMap: mockSetMap, + setIcon: mockSetIcon, + setPosition: mockSetPosition, + setTitle: mockSetTitle, + addListener: mockAddListener, + })), + Point: jest.fn().mockImplementation((x, y) => ({ x, y })), + event: { + clearListeners: jest.fn(), + removeListener: mockRemoveListener, + }, + }, +}; + +describe("Map Widget - Marker Component", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("should trigger onClick callback only once per click", () => { + const onClickMock = jest.fn(); + + // Render the marker component with onClick handler + render( + , + ); + + // Simulate a marker click by directly calling the stored callback + ( + mockAddListener as unknown as { + clickCallback: (...args: unknown[]) => void; + } + ).clickCallback(); + + // Verify onClick was called exactly once + expect(onClickMock).toHaveBeenCalledTimes(1); + + // Simulate another click + ( + mockAddListener as unknown as { + clickCallback: (...args: unknown[]) => void; + } + ).clickCallback(); + + // Verify onClick was called exactly twice (once per click) + expect(onClickMock).toHaveBeenCalledTimes(2); + }); + + it("should clear previous click listeners when onClick prop changes", () => { + const { rerender } = render( + , + ); + + // Verify clearListeners was called during initial render + expect(google.maps.event.clearListeners).toHaveBeenCalledWith( + expect.anything(), + "click", + ); + + // Reset the mock to check if it's called again + (google.maps.event.clearListeners as jest.Mock).mockClear(); + + // Rerender with a different onClick handler + rerender( + , + ); + + // Verify clearListeners was called again when onClick changed + expect(google.maps.event.clearListeners).toHaveBeenCalledWith( + expect.anything(), + "click", + ); + }); +}); diff --git a/app/client/src/widgets/MapWidget/component/Marker.tsx b/app/client/src/widgets/MapWidget/component/Marker.tsx index 7701ded5be..ef9bec51e4 100644 --- a/app/client/src/widgets/MapWidget/component/Marker.tsx +++ b/app/client/src/widgets/MapWidget/component/Marker.tsx @@ -31,10 +31,6 @@ const Marker: React.FC = (options) => { title, }); - googleMapMarker.addListener("click", () => { - if (onClick) onClick(); - }); - setMarker(googleMapMarker); } @@ -79,19 +75,32 @@ const Marker: React.FC = (options) => { useEffect(() => { if (!marker) return; - marker.addListener("click", () => { + google.maps.event.clearListeners(marker, "click"); + const clickListener = marker.addListener("click", () => { if (onClick) onClick(); }); + + return () => { + google.maps.event.removeListener(clickListener); + }; }, [marker, onClick]); // add dragend event on marker useEffect(() => { - if (!marker) return; + if (!marker || !onDragEnd) return; - marker.addListener("dragend", (e: google.maps.MapMouseEvent) => { - if (onDragEnd) onDragEnd(e); - }); - }, [marker, options.onDragEnd]); + google.maps.event.clearListeners(marker, "dragend"); + const dragEndListener = marker.addListener( + "dragend", + (e: google.maps.MapMouseEvent) => { + if (onDragEnd) onDragEnd(e); + }, + ); + + return () => { + google.maps.event.removeListener(dragEndListener); + }; + }, [marker, onDragEnd]); return null; };