From c1f7587704642dd8f5326a1d8d7daa9aeed5d151 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 5 Sep 2026 01:10:33 +0200 Subject: [PATCH] fix(roadtrip): a freshly placed via lost its first drag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Placing one triggers a re-route, the re-route lands about a second later, and the render it causes rebuilt every via handle — which is exactly when somebody reaches for the dot they just made. On the GL renderer that rebuild replaces the DOM element outright, 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, and an older via never showed it because nothing was re-routing. Two causes, both identity: the vias arrive as a fresh object on every reload even when the rows came back identical, and `onMoveVia` is a fresh closure on every render of the planner. Both were in the effect's dependencies. The list is now keyed by what a handle is built from — id and position — so it only changes when a via does, and the callbacks are read through a ref. A via that genuinely moved still gets a new list, which is what redraws it where it now is; the test pins both halves, because holding the element still must not mean holding it in the wrong place. Leaflet's markers had the same churn without the same symptom: a fresh `position` array called `setLatLng` on every marker on every render, and a fresh `eventHandlers` object unbound and rebound every listener. Same fix. --- client/src/components/Map/MapViewGL.test.tsx | 71 +++++++++++++++++++ client/src/components/Map/MapViewGL.tsx | 27 +++++-- .../src/components/Map/RoadtripViaMarkers.tsx | 54 +++++++++----- .../src/components/Map/viaMarkerState.test.ts | 68 ++++++++++++++++++ client/src/components/Map/viaMarkerState.ts | 30 ++++++++ 5 files changed, 226 insertions(+), 24 deletions(-) create mode 100644 client/src/components/Map/viaMarkerState.test.ts create mode 100644 client/src/components/Map/viaMarkerState.ts 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 +}