diff --git a/client/src/components/Map/MapViewGL.test.tsx b/client/src/components/Map/MapViewGL.test.tsx index e0022f6eb..784c60060 100644 --- a/client/src/components/Map/MapViewGL.test.tsx +++ b/client/src/components/Map/MapViewGL.test.tsx @@ -1884,4 +1884,75 @@ describe('MapViewGL', () => { expect(pin.style.transform).toContain('translate(140px, 60px)') }) + + // A via handle is a DOM element, and a pointerdown belongs to the element it happened + // on. Rebuilding the handles on an unrelated render pulled that element out from under + // the pointer, so a freshly placed via lost its first drag — the re-route it triggered + // landed about a second later, which is exactly when somebody reaches for it. + it('FE-COMP-MAPVIEWGL-VIA-001: a via handle survives a render it has nothing to do with', async () => { + loadOnAttach() + glCanvasContainer.replaceChildren() + glMap.project.mockReturnValue({ x: 100, y: 80 }) + + const stored = { id: 5, day_id: 1, after_order_index: 0, sequence: 0, lat: 48.1, lng: 2.1 } + const { rerender } = render( + {}} + />, + ) + await flushFrames() + + const layer = glCanvasContainer.firstElementChild as HTMLElement + const handle = layer.firstElementChild + expect(handle).toBeTruthy() + + // The same via in a fresh object, with a fresh callback: what every render of the + // planner hands down while the day re-routes. + rerender( + {}} + />, + ) + await flushFrames() + + expect(layer.firstElementChild).toBe(handle) + }) + + it('FE-COMP-MAPVIEWGL-VIA-002: a via that actually moved is drawn again', async () => { + loadOnAttach() + glCanvasContainer.replaceChildren() + glMap.project.mockReturnValue({ x: 100, y: 80 }) + + const stored = { id: 5, day_id: 1, after_order_index: 0, sequence: 0, lat: 48.1, lng: 2.1 } + const { rerender } = render( + {}} />, + ) + await flushFrames() + + const layer = glCanvasContainer.firstElementChild as HTMLElement + const handle = layer.firstElementChild + + rerender( + {}} + />, + ) + await flushFrames() + + // The other half of the contract: holding the element still must not mean holding it + // in the wrong place. + expect(layer.firstElementChild).not.toBe(handle) + }) }) diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index 98293c4e7..962c1bb4f 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -1,6 +1,7 @@ import { useEffect, useRef, useMemo, useState, createElement, useCallback } from 'react' import { makeMarkerDraggable, makePoiDraggable, draggedPoiId } from './markerDrag' import type { RoadtripVia } from '@trek/shared' +import { useStableVias } from './viaMarkerState' import { ALT_CASING, ALT_LABEL_TEXT } from '../Roadtrip/alternativeColors' import type { AlternativeOverlay } from '../Roadtrip/alternativeOverlays' import { serviceMarkerHtml, serviceMarkerOuter } from '../Roadtrip/serviceMarker' @@ -678,6 +679,19 @@ export function MapViewGL({ * drag is therefore hand-rolled — pointer events on the element, unproject on move. */ const viaCleanupRef = useRef<(() => void)[]>([]) + /** + * The list only changes when a via does, and the callbacks are read through a ref. + * + * Both matter for the same reason: this effect destroys every handle and builds new DOM + * elements. It used to re-run on every render of the planner — the vias arrived as a + * fresh object each time and `onMoveVia` as a fresh closure — so the render that landed + * when a freshly placed via finished re-routing pulled the element out from under the + * pointer, and the first drag went nowhere. + */ + const vias = useStableVias(roadtripVias) + const viaHandlersRef = useRef({ onMoveVia, onRemoveVia }) + viaHandlersRef.current = { onMoveVia, onRemoveVia } + const viasDraggable = !!onMoveVia useEffect(() => { const map = mapRef.current if (!map || !mapReady) return @@ -685,12 +699,12 @@ export function MapViewGL({ viaCleanupRef.current = [] viaPinsRef.current.forEach(p => p.remove()) viaPinsRef.current = [] - for (const via of Object.values(roadtripVias ?? {}).flat()) { + for (const via of vias) { const el = document.createElement('span') el.style.cssText = 'display:block;width:12px;height:12px;border-radius:9999px;background:#0a84ff;border:2.5px solid #fff;box-shadow:0 1px 4px rgba(0,0,0,.45);cursor:grab;touch-action:none' const pin = attachPin(map, gl, pinLayerRef.current, el, via.lng, via.lat) viaPinsRef.current.push(pin) - if (!onMoveVia) continue + if (!viasDraggable) continue let dragging = false // The map starts panning on mousedown/touchstart anywhere on its surface, and a pin @@ -716,9 +730,12 @@ export function MapViewGL({ el.style.cursor = 'grab' const rect = map.getContainer().getBoundingClientRect() const at = map.unproject([e.clientX - rect.left, e.clientY - rect.top]) - onMoveVia(via.day_id, via.id, at.lat, at.lng) + viaHandlersRef.current.onMoveVia?.(via.day_id, via.id, at.lat, at.lng) + } + const onContext = (e: MouseEvent) => { + e.preventDefault() + viaHandlersRef.current.onRemoveVia?.(via.day_id, via.id) } - const onContext = (e: MouseEvent) => { e.preventDefault(); onRemoveVia?.(via.day_id, via.id) } el.addEventListener('pointerdown', onDown) el.addEventListener('pointermove', onMove) el.addEventListener('pointerup', onUp) @@ -738,7 +755,7 @@ export function MapViewGL({ el.removeEventListener('dblclick', swallow) }) } - }, [roadtripVias, mapReady, glProvider, onMoveVia, onRemoveVia]) + }, [vias, viasDraggable, mapReady, glProvider]) /** * The offered routes, drawn under the current one. diff --git a/client/src/components/Map/RoadtripViaMarkers.tsx b/client/src/components/Map/RoadtripViaMarkers.tsx index 7ee93f6f2..fb6cab5ff 100644 --- a/client/src/components/Map/RoadtripViaMarkers.tsx +++ b/client/src/components/Map/RoadtripViaMarkers.tsx @@ -1,7 +1,8 @@ -import React, { useMemo } from 'react' +import React, { useMemo, useRef } from 'react' import { Marker, Tooltip } from 'react-leaflet' import L from 'leaflet' import { useTranslation } from '../../i18n/TranslationContext' +import { useStableVias } from './viaMarkerState' import type { RoadtripVia } from '@trek/shared' /** @@ -29,35 +30,50 @@ interface RoadtripViaMarkersProps { onRemoveVia?: (dayId: number, id: number) => void } -/** Leaflet's half; the GL renderers draw their own in `RoadtripViaMarkersGL`. */ +/** Leaflet's half; the GL renderers draw their own inside `MapViewGL`. */ export default function RoadtripViaMarkers({ viasByDay, onMoveVia, onRemoveVia, }: RoadtripViaMarkersProps): React.ReactElement | null { const { t } = useTranslation() - const vias = useMemo(() => Object.values(viasByDay).flat(), [viasByDay]) - if (!vias.length) return null + const vias = useStableVias(viasByDay) + // Read through a ref so the handlers below can stay the same objects across renders. + // Both callbacks are rebuilt on every render of the planner, and react-leaflet compares + // its props by identity: a fresh `eventHandlers` unbinds and rebinds every listener, + // and a fresh `position` array calls `setLatLng` on every marker. Doing either while a + // drag is in flight is how a freshly placed via lost its first drag. + const handlersRef = useRef({ onMoveVia, onRemoveVia }) + handlersRef.current = { onMoveVia, onRemoveVia } + const draggable = !!onMoveVia + + const markers = useMemo(() => vias.map(via => ({ + via, + position: [via.lat, via.lng] as [number, number], + handlers: { + dragend: (e: { target: { getLatLng: () => { lat: number; lng: number } } }) => { + const at = e.target.getLatLng() + handlersRef.current.onMoveVia?.(via.day_id, via.id, at.lat, at.lng) + }, + // Right-click rather than a delete handle: a 12px dot has no room for one, + // and the same gesture removes things elsewhere on the map. + contextmenu: (e: { originalEvent: MouseEvent }) => { + e.originalEvent.preventDefault() + handlersRef.current.onRemoveVia?.(via.day_id, via.id) + }, + }, + })), [vias]) + + if (!markers.length) return null return ( <> - {vias.map(via => ( + {markers.map(({ via, position, handlers }) => ( { lat: number; lng: number } } }) => { - const at = e.target.getLatLng() - onMoveVia?.(via.day_id, via.id, at.lat, at.lng) - }, - // Right-click rather than a delete handle: a 12px dot has no room for one, - // and the same gesture removes things elsewhere on the map. - contextmenu: (e: { originalEvent: MouseEvent }) => { - e.originalEvent.preventDefault() - onRemoveVia?.(via.day_id, via.id) - }, - }} + eventHandlers={handlers} > {t('roadtrip.via.hint')} diff --git a/client/src/components/Map/viaMarkerState.test.ts b/client/src/components/Map/viaMarkerState.test.ts new file mode 100644 index 000000000..0d5dc4613 --- /dev/null +++ b/client/src/components/Map/viaMarkerState.test.ts @@ -0,0 +1,68 @@ +import { renderHook } from '@testing-library/react' +import { describe, it, expect } from 'vitest' +import { useStableVias } from './viaMarkerState' +import type { RoadtripVia } from '@trek/shared' + +/** + * The list both map renderers rebuild their via handles from. + * + * Its identity is the whole contract: the GL renderer replaces the DOM element when this + * changes, and a pointerdown belongs to the element it happened on. A list that changed + * on every render meant a freshly placed via lost its first drag to the re-route landing + * underneath it. + */ +const via = (over: Partial & { id: number }): RoadtripVia => ({ + day_id: 1, after_order_index: 0, sequence: 0, lat: 48.1, lng: 2.1, ...over, +}) + +describe('useStableVias', () => { + it('FE-VIASTATE-001: the same vias in a fresh object are the same list', () => { + // Exactly what a reload hands down: identical rows, a new object every time. + const { result, rerender } = renderHook( + ({ byDay }) => useStableVias(byDay), + { initialProps: { byDay: { 1: [via({ id: 5 })] } } }, + ) + const first = result.current + + rerender({ byDay: { 1: [via({ id: 5 })] } }) + expect(result.current).toBe(first) + }) + + it('FE-VIASTATE-002: a via that moved is a new list, so the handle is redrawn where it now is', () => { + const { result, rerender } = renderHook( + ({ byDay }) => useStableVias(byDay), + { initialProps: { byDay: { 1: [via({ id: 5 })] } } }, + ) + const first = result.current + + rerender({ byDay: { 1: [via({ id: 5, lat: 48.9 })] } }) + expect(result.current).not.toBe(first) + expect(result.current[0].lat).toBe(48.9) + }) + + it('FE-VIASTATE-003: adding or removing one is a new list too', () => { + const { result, rerender } = renderHook( + ({ byDay }) => useStableVias(byDay), + { initialProps: { byDay: { 1: [via({ id: 5 })] } } }, + ) + const first = result.current + + rerender({ byDay: { 1: [via({ id: 5 }), via({ id: 6, lat: 48.4 })] } }) + expect(result.current).not.toBe(first) + expect(result.current).toHaveLength(2) + }) + + it('FE-VIASTATE-004: every day of the trip is in one list, and no vias is a stable empty one', () => { + const { result, rerender } = renderHook( + ({ byDay }) => useStableVias(byDay), + { initialProps: { byDay: { 1: [via({ id: 5 })], 2: [via({ id: 6, day_id: 2 })] } as Record | undefined }, + }) + expect(result.current.map(v => v.id)).toEqual([5, 6]) + + rerender({ byDay: undefined }) + const empty = result.current + expect(empty).toEqual([]) + rerender({ byDay: {} }) + expect(result.current).toBe(empty) + }) +}) diff --git a/client/src/components/Map/viaMarkerState.ts b/client/src/components/Map/viaMarkerState.ts new file mode 100644 index 000000000..00d26d40b --- /dev/null +++ b/client/src/components/Map/viaMarkerState.ts @@ -0,0 +1,30 @@ +import { useRef } from 'react' +import type { RoadtripVia } from '@trek/shared' + +/** + * The trip's via points as an array that only changes when a via does. + * + * Both map renderers rebuild their via handles whenever this list changes, and the GL one + * replaces the DOM element outright. The list it was given changed on every render: the + * hook that loads vias returns a fresh object each time, and a fresh object arrives again + * on every reload even when the rows came back identical. + * + * That made dragging a freshly placed via fail on the first try. Placing one triggers a + * re-route, the re-route lands about a second later, the render it causes replaced the + * handle under the pointer — and a pointerdown belongs to the element it happened on, so + * the drag went to an element that no longer existed. The second attempt worked because + * by then nothing was pending. An older via never showed it: nothing was re-routing. + * + * Keyed by id and position because those are the only fields a handle is built from. A + * via that genuinely moved gets a new list, which is what redraws it where it now is. + */ +export function useStableVias(viasByDay: Record | undefined): RoadtripVia[] { + const flat = Object.values(viasByDay ?? {}).flat() + const key = flat.map(v => `${v.id}@${v.lat},${v.lng}`).join('|') + // Read and written during render on purpose: this is a cache of the argument, not + // state — it never triggers a render of its own, and it has to be settled before the + // effects that consume the list run. + const held = useRef<{ key: string; list: RoadtripVia[] }>({ key: '', list: [] }) + if (held.current.key !== key) held.current = { key, list: flat } + return held.current.list +}