From 12eabbb6856bc03a24151bb65cf158d325079dc6 Mon Sep 17 00:00:00 2001 From: Wenyin Wei Date: Sun, 13 Sep 2026 01:11:14 +0800 Subject: [PATCH] fix(shared): cluster the public share page's markers The share page drew every stop as its own pin while the planner drew the same trip as a handful of clusters, so a shared link opened on three dozen overlapping markers. On a 300px-tall map that covers a whole city, the pins underneath can be neither read nor tapped (#2343). The option block and the icon factory move to components/Map/markerCluster.ts, which both maps import, so the two cannot drift apart again. The planner's options are unchanged. leaflet.markercluster's two stylesheets were imported by MapView.tsx alone. The share page never mounts MapView, so it imports them itself: without them the bubbles keep the library's default styling and spiderfy's legs are invisible. --- client/src/components/Map/MapView.tsx | 24 +-- client/src/components/Map/markerCluster.ts | 44 ++++++ client/src/pages/SharedTripPage.test.tsx | 10 ++ client/src/pages/SharedTripPage.tsx | 23 ++- .../unit/pages/SharedTripPage.map.test.tsx | 138 ++++++++++++++++++ 5 files changed, 212 insertions(+), 27 deletions(-) create mode 100644 client/src/components/Map/markerCluster.ts create mode 100644 client/tests/unit/pages/SharedTripPage.map.test.tsx diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 276d2c049..5f8390409 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -4,6 +4,7 @@ import { renderIconMarkup } from '../../utils/iconMarkup' import { MapContainer, TileLayer, Marker, Polyline, CircleMarker, Circle, useMap, Tooltip } from 'react-leaflet' import MarkerClusterGroup from 'react-leaflet-cluster' import { makeMarkerDraggable, makePoiDraggable, draggedPoiId } from './markerDrag' +import { CLUSTER_OPTIONS, createClusterIcon } from './markerCluster' import RoadtripViaMarkers from './RoadtripViaMarkers' import HazardLayers from './HazardLayers' import { ALT_CASING, ALT_LABEL_TEXT } from '../Roadtrip/alternativeColors' @@ -868,16 +869,6 @@ export const MapView = memo(function MapView({ } }, [placeIds, placesPhotosEnabled]) - const clusterIconCreateFunction = useCallback((cluster) => { - const count = cluster.getChildCount() - const size = count < 10 ? 36 : count < 50 ? 42 : 48 - return L.divIcon({ - html: `
${count}
`, - className: 'marker-cluster-wrapper', - iconSize: L.point(size, size), - }) - }, []) - const isTouchDevice = typeof window !== 'undefined' && navigator.maxTouchPoints > 0 // Drag a marker onto a day (#891). Pointer-driven, so it is off wherever // HTML5 drag does not exist — and the day plan is not on screen there anyway. @@ -1061,18 +1052,7 @@ export const MapView = memo(function MapView({ - + {markers} diff --git a/client/src/components/Map/markerCluster.ts b/client/src/components/Map/markerCluster.ts new file mode 100644 index 000000000..b831b8b39 --- /dev/null +++ b/client/src/components/Map/markerCluster.ts @@ -0,0 +1,44 @@ +import L from 'leaflet' + +/** + * The cluster group every place map is drawn with. + * + * The planner and the public share page draw the same trip, so they have to + * cluster it the same way: a shared link that opens on three dozen overlapping + * pins while the planner shows four tidy bubbles is one trip told two different + * ways. Both maps build their group from this block and this factory (#2343). + */ +export const CLUSTER_OPTIONS = { + chunkedLoading: true, + chunkInterval: 30, + chunkDelay: 0, + // Pixels at the current zoom, so ordinary stops still come apart by zooming in. + maxClusterRadius: 20, + disableClusteringAtZoom: 9, + spiderfyOnMaxZoom: true, + showCoverageOnHover: false, + zoomToBoundsOnClick: true, + animate: false, +} + +/** The part of a Leaflet cluster this factory reads. */ +export interface ClusterLike { + getChildCount: () => number +} + +/** + * A count bubble, sized by what it stands for. + * + * Markup rather than a component because Leaflet owns this element: + * `L.divIcon`'s html lands in the marker pane, outside React's tree — which is + * also why its styling lives in index.css (`.marker-cluster-custom`). + */ +export function createClusterIcon(cluster: ClusterLike) { + const count = cluster.getChildCount() + const size = count < 10 ? 36 : count < 50 ? 42 : 48 + return L.divIcon({ + html: `
${count}
`, + className: 'marker-cluster-wrapper', + iconSize: L.point(size, size), + }) +} diff --git a/client/src/pages/SharedTripPage.test.tsx b/client/src/pages/SharedTripPage.test.tsx index 68efc409a..30bf9db74 100644 --- a/client/src/pages/SharedTripPage.test.tsx +++ b/client/src/pages/SharedTripPage.test.tsx @@ -24,6 +24,16 @@ vi.mock('react-leaflet', () => ({ }), })); +// The markers now reach the map through a cluster group (#2343). The real wrapper +// asks React Leaflet's context for the map it belongs to, and the stubbed +// MapContainer above provides none — so it is stubbed for the same reason +// everything else here is. +vi.mock('react-leaflet-cluster', () => ({ + default: ({ children }: { children?: React.ReactNode }) => ( +
{children}
+ ), +})); + // The basemap is a MapLibre style now, and the real component reaches for // maplibre-gl through a dynamic import. The page test only cares that it is the // thing being rendered. diff --git a/client/src/pages/SharedTripPage.tsx b/client/src/pages/SharedTripPage.tsx index c9dadaf61..ba00b68ad 100644 --- a/client/src/pages/SharedTripPage.tsx +++ b/client/src/pages/SharedTripPage.tsx @@ -18,6 +18,13 @@ import { import { createElement, useEffect, useRef } from 'react'; import { renderIconMarkup } from '../utils/iconMarkup'; import { MapContainer, Marker, Polyline, TileLayer, Tooltip, useMap } from 'react-leaflet'; +import MarkerClusterGroup from 'react-leaflet-cluster'; +// leaflet.markercluster's own sheets travel with the planner's MapView, which this +// page never mounts: without them the bubbles keep the library's default styling +// and spiderfy's legs are invisible. +import 'leaflet.markercluster/dist/MarkerCluster.css'; +import 'leaflet.markercluster/dist/MarkerCluster.Default.css'; +import { CLUSTER_OPTIONS, createClusterIcon } from '../components/Map/markerCluster'; import { getCategoryIcon } from '../components/shared/categoryIcons'; import PublicLanguagePicker from '../components/shared/PublicLanguagePicker'; import { OFM_POSITRON, DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM, MAP_MAX_ZOOM, attributionForTile } from '../constants/mapDefaults'; @@ -454,11 +461,17 @@ export default function SharedTripPage() { interactive={false} /> )} - {mapPlaces.map((p: any) => ( - - {p.name} - - ))} + {/* Clustered like the planner's map (#2343). A shared link draws the + whole trip at once on a 300px-tall map, where a stop a few blocks + from the next one is the same few pixels: unclustered, the pins + cover each other and the ones underneath cannot be tapped. */} + + {mapPlaces.map((p: any) => ( + + {p.name} + + ))} + diff --git a/client/tests/unit/pages/SharedTripPage.map.test.tsx b/client/tests/unit/pages/SharedTripPage.map.test.tsx new file mode 100644 index 000000000..83a213d14 --- /dev/null +++ b/client/tests/unit/pages/SharedTripPage.map.test.tsx @@ -0,0 +1,138 @@ +/** + * The share page's map is the planner's map (#2343). + * + * A shared link is the only view a guest ever gets — no account, no zoom habits, + * often a phone — and it draws the whole trip at once on a 300px-tall canvas. + * Unclustered, three dozen pins pile onto each other there and the ones + * underneath can be neither read nor tapped, while the planner shows the same + * trip as a handful of tidy bubbles. + * + * The data hooks and the Leaflet components are stubbed: what is under test is + * the page's own wiring — that every stop reaches a cluster group built from the + * shared option block — not Leaflet's rendering, which jsdom cannot do anyway. + */ +import React from 'react'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { render, screen, within } from '../../helpers/render'; +import { CLUSTER_OPTIONS, createClusterIcon } from '../../../src/components/Map/markerCluster'; +import SharedTripPage from '../../../src/pages/SharedTripPage'; + +const mocks = vi.hoisted(() => ({ + hook: {} as Record, + clusterProps: [] as Array>, +})); + +vi.mock('../../../src/pages/sharedTrip/useSharedTrip', () => ({ + useSharedTrip: () => mocks.hook, +})); + +vi.mock('react-leaflet', () => ({ + MapContainer: ({ children }: { children?: React.ReactNode }) =>
{children}
, + TileLayer: () => null, + Polyline: () => null, + Tooltip: ({ children }: { children?: React.ReactNode }) => {children}, + Marker: ({ children }: { children?: React.ReactNode }) =>
{children}
, + useMap: () => ({ fitBounds: vi.fn(), on: vi.fn(), off: vi.fn() }), +})); + +vi.mock('react-leaflet-cluster', () => ({ + default: (props: { children?: React.ReactNode }) => { + mocks.clusterProps.push(props as unknown as Record); + return
{props.children}
; + }, +})); + +// Its effect dynamically imports maplibre-gl, which has no business in jsdom. +vi.mock('../../../src/components/Map/VectorBasemap', () => ({ default: () => null })); + +const place = (id: number, name: string, lat: number, lng: number) => ({ + id, + name, + lat, + lng, + category_color: '#6366f1', + category_icon: 'map-pin', +}); + +function payload(places: Array>) { + return { + trip: { id: 1, title: 'Autumn', start_date: '2026-09-25', end_date: '2026-09-27', currency: 'EUR' }, + days: [{ id: 5, day_number: 1, date: '2026-09-25', title: 'Day one' }], + assignments: {}, + dayNotes: {}, + places, + reservations: [], + accommodations: [], + packing: [], + budget: [], + categories: [], + permissions: { share_map: true, share_bookings: true, share_packing: true, share_budget: true }, + collab: null, + cartoApiKey: null, + }; +} + +function hookState(data: unknown) { + return { + data, + error: false, + base: 'EUR', + convert: (n: number) => n, + selectedDay: null, + setSelectedDay: vi.fn(), + activeTab: 'plan', + setActiveTab: vi.fn(), + showLangPicker: false, + setShowLangPicker: vi.fn(), + }; +} + +beforeEach(() => { + mocks.clusterProps.length = 0; +}); + +describe('the map on a public share page', () => { + it('SHAREMAP-001: every stop is handed to a cluster group, not the raw map', () => { + mocks.hook = hookState( + payload([place(11, 'The Bund', 31.24, 121.49), place(12, 'Peace Hotel', 31.2397, 121.4903)]), + ); + render(); + + const cluster = screen.getByTestId('cluster-group'); + expect(within(cluster).getAllByTestId('marker')).toHaveLength(2); + // And nothing escaped it: the map holds no bare markers of its own. + expect(within(screen.getByTestId('map')).getAllByTestId('marker')).toHaveLength(2); + }); + + it('SHAREMAP-002: the group is built from the shared options, so both maps cluster alike', () => { + mocks.hook = hookState(payload([place(11, 'The Bund', 31.24, 121.49)])); + render(); + + expect(mocks.clusterProps).toHaveLength(1); + const props = mocks.clusterProps[0]; + for (const [key, value] of Object.entries(CLUSTER_OPTIONS)) { + expect(props[key], key + ' must reach the cluster group').toBe(value); + } + expect(typeof props.iconCreateFunction).toBe('function'); + }); + + it('SHAREMAP-004: the bubble shows the count, in the size bucket that count earns', () => { + const small = createClusterIcon({ getChildCount: () => 7 }); + const medium = createClusterIcon({ getChildCount: () => 30 }); + const large = createClusterIcon({ getChildCount: () => 60 }); + + expect(small.options.className).toBe('marker-cluster-wrapper'); + expect(small.options.html).toContain('7'); + expect([small, medium, large].map((icon) => icon.options.iconSize?.x)).toEqual([36, 42, 48]); + expect(small.options.html).toContain('width:36px;height:36px'); + }); + + it('SHAREMAP-003: a place without coordinates is left out of the map, as before', () => { + mocks.hook = hookState( + payload([place(11, 'The Bund', 31.24, 121.49), { id: 12, name: 'Geocoding failed' } as never]), + ); + render(); + + expect(within(screen.getByTestId('cluster-group')).getAllByTestId('marker')).toHaveLength(1); + }); +});