Draw tracks in their own colour on both map engines

The MapLibre layer was already data-driven, so it only needed the feature property filled. Leaflet needed more work: the GPX polyline passed its style as bare color/weight props, and react-leaflet only calls setStyle when the pathOptions reference changes — a recoloured track would have kept its mount-time colour until the next reload. Every other vector layer in here already uses pathOptions. A track with a colour gets a white casing so it survives satellite imagery and dark basemaps. The casings live in their own pane below the lines: Leaflet stacks paths in insertion order, so drawn inline they end up over the line of a track that was added earlier. They are always mounted and hidden by opacity, otherwise giving a track a colour remounts the path into the wrong position. Tracks are clickable now — 3.5px is not a target, so the hit area is a transparent 14px line on top. bubblingMouseEvents is off on it: paths bubble to the map by default (markers do not), and the map's own click handler would clear the selection the track just made. The GL side needs the mirror image of that, a guard so a click on a cluster bubble over a track still belongs to the cluster.
2026-10-19 13:36:48 +00:00 · 2026-07-27 17:35:41 +02:00
parent 1b5317a817
commit fcabb4c645
6 changed files with 288 additions and 12 deletions
@@ -40,7 +40,22 @@ vi.mock('react-leaflet', () => ({
      {children}
    </div>
  ),
-  Polyline: ({ positions }: any) => <div data-testid="polyline" data-points={JSON.stringify(positions)} />,
+  // pathOptions has to reach the DOM: the style props are what the track-colour
+  // feature (#776) actually asserts on, and the real Leaflet only repaints when
+  // that object's reference changes.
+  // bubblingMouseEvents and pane are surfaced too: a jsdom mock cannot reproduce
+  // Leaflet's event propagation or its pane stacking, so the tests assert the
+  // options that govern them instead.
+  Polyline: ({ positions, pathOptions, eventHandlers, bubblingMouseEvents, pane }: any) => (
+    <div
+      data-testid="polyline"
+      data-points={JSON.stringify(positions)}
+      data-path-options={JSON.stringify(pathOptions ?? null)}
+      data-bubbling={String(bubblingMouseEvents)}
+      data-pane={pane ?? ''}
+      onClick={() => eventHandlers?.click?.()}
+    />
+  ),
  CircleMarker: () => <div data-testid="circle-marker" />,
  Circle: () => <div data-testid="circle" />,
  Tooltip: ({ children }: any) => <>{children}</>,
@@ -154,7 +169,10 @@ describe('MapView', () => {
      buildMapPlace({ lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]' }),
    ]
    render(<MapView places={places} />)
-    expect(screen.getByTestId('polyline')).toBeTruthy()
+    // Three per track: casing, the visible line, and the invisible fat one
+    // that catches clicks. The casing is always mounted so that toggling a
+    // colour never remounts a path into the wrong stacking position.
+    expect(screen.getAllByTestId('polyline').length).toBe(3)
  })

  it('FE-COMP-MAPVIEW-010: MarkerClusterGroup is rendered', () => {
@@ -187,6 +205,67 @@ describe('MapView', () => {
    expect(screen.queryByTestId('polyline')).toBeNull()
  })

+  // ── Track colours (#776) ──────────────────────────────────────────────────
+  // The style has to travel through pathOptions: react-leaflet only calls
+  // setStyle when that object's reference changes, so bare color/weight props
+  // would freeze at their mount-time value and a recolour would never show.
+  const trackOptions = () => screen.getAllByTestId('polyline')
+    .map(el => JSON.parse(el.getAttribute('data-path-options') || 'null'))
+    .filter(Boolean)
+
+  it('FE-COMP-MAPVIEW-025: a picked route_color beats the category colour', () => {
+    const places = [buildMapPlace({
+      lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]',
+      category_color: '#00ff00', route_color: '#e11d48',
+    })]
+    render(<MapView places={places} />)
+    expect(trackOptions().some(o => o.color === '#e11d48')).toBe(true)
+    expect(trackOptions().some(o => o.color === '#00ff00')).toBe(false)
+  })
+
+  it('FE-COMP-MAPVIEW-026: without route_color the category colour still wins', () => {
+    const places = [buildMapPlace({
+      lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]',
+      category_color: '#00ff00', route_color: null,
+    })]
+    render(<MapView places={places} />)
+    expect(trackOptions().some(o => o.color === '#00ff00')).toBe(true)
+  })
+
+  it('FE-COMP-MAPVIEW-027: with neither colour the track keeps the old blue', () => {
+    const places = [buildMapPlace({ lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]' })]
+    render(<MapView places={places} />)
+    expect(trackOptions().some(o => o.color === '#3b82f6' && o.weight === 3.5 && o.opacity === 0.75)).toBe(true)
+    // The casing is mounted but invisible — a track nobody coloured looks
+    // exactly as it did before.
+    expect(trackOptions().some(o => o.color === '#ffffff' && o.opacity === 0)).toBe(true)
+    expect(trackOptions().some(o => o.color === '#ffffff' && o.opacity > 0)).toBe(false)
+  })
+
+  it('FE-COMP-MAPVIEW-028: a coloured track gets a white casing underneath', () => {
+    const places = [buildMapPlace({
+      lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]', route_color: '#059669',
+    })]
+    render(<MapView places={places} />)
+    expect(trackOptions().some(o => o.color === '#ffffff' && o.weight === 6.5 && o.opacity === 0.7)).toBe(true)
+    expect(trackOptions().some(o => o.color === '#059669' && o.opacity === 0.9)).toBe(true)
+  })
+
+  it('FE-COMP-MAPVIEW-029: clicking a track selects its place', () => {
+    const onMarkerClick = vi.fn()
+    const places = [buildMapPlace({
+      id: 77, lat: 48.0, lng: 2.0, route_geometry: '[[48.0,2.0],[49.0,3.0]]',
+    })]
+    render(<MapView places={places} onMarkerClick={onMarkerClick} />)
+    const hit = screen.getAllByTestId('polyline')
+      .find(el => JSON.parse(el.getAttribute('data-path-options') || '{}').weight === 14)
+    fireEvent.click(hit!)
+    expect(onMarkerClick).toHaveBeenCalledWith(77)
+    // Paths bubble to the map by default and the map click clears the selection
+    // this one just made — the mock cannot reproduce that, so assert the option.
+    expect(hit!.getAttribute('data-bubbling')).toBe('false')
+  })
+
  it('FE-COMP-MAPVIEW-014: marker icon uses base64 image_url for photo places', () => {
    const dataUrl = 'data:image/jpeg;base64,/9j/4AA'
    const places = [buildMapPlace({ id: 10, lat: 48.0, lng: 2.0, image_url: dataUrl })]
@@ -15,6 +15,7 @@ import { useTransportRoutes } from '../../hooks/useTransportRoutes'
 import { visibleRouteReservations } from '../../utils/reservationRoutes'
 import type { Reservation, RouteVia } from '../../types'
 import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories'
+import { resolveTrackColor, hasManualTrackColor } from './trackColors'
 import { DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults'
 import { computeMapViewport, TILE_SIZE_RASTER, type ViewportPadding } from '../../utils/mapViewport'

@@ -338,6 +339,28 @@ function ZoomTracker({ onZoomStart, onZoomEnd }: { onZoomStart: () => void; onZo
  return null
 }

+const TRACK_CASING_PANE = 'trek-track-casing'
+
+/**
+ * Pane that holds the white casing under GPX tracks (#776). Leaflet stacks paths
+ * in insertion order, so without a pane of its own a casing mounted later — a
+ * newly imported track, or one that just got a colour — would paint over an
+ * earlier track's line. Pane support is an optimization: a renderer without the
+ * pane API still draws everything, just in insertion order.
+ */
+function TrackCasingPane({ onReady }: { onReady: (ready: boolean) => void }) {
+  const map = useMap()
+  useEffect(() => {
+    if (typeof map.getPane !== 'function' || typeof map.createPane !== 'function') return
+    if (!map.getPane(TRACK_CASING_PANE)) {
+      const pane = map.createPane(TRACK_CASING_PANE)
+      if (pane) pane.style.zIndex = '398' // under overlayPane (400) and the plugin pane (399)
+    }
+    onReady(true)
+  }, [map, onReady])
+  return null
+}
+
 function MapClickHandler({ onClick }: MapClickHandlerProps) {
  const map = useMap()
  useEffect(() => {
@@ -574,6 +597,8 @@ export const MapView = memo(function MapView({
    return () => window.removeEventListener('scroll', clear, true)
  }, [hoveredPlace])

+  const [hasCasingPane, setHasCasingPane] = useState(false)
+
  const handleMarkerClick = useCallback((id: number) => {
    // Clear the hover card right away: the recenter that follows moves the
    // marker out from under the cursor, so no mouseout will ever fire (#1404).
@@ -686,22 +711,66 @@ export const MapView = memo(function MapView({
    )
  }), [places, selectedPlaceId, dayOrderMap, photoUrls, handleMarkerClick, handleMarkerHover, handleMarkerHoverOut])

-  const gpxPolylines = useMemo(() => places.flatMap(place => {
+  // Parsing track geometry is the expensive part (tracks run to tens of thousands
+  // of points), so it hangs off `places` alone — a selection change must not
+  // re-parse every track.
+  const gpxTracks = useMemo(() => places.flatMap(place => {
    if (!place.route_geometry) return []
    try {
      const coords = JSON.parse(place.route_geometry) as [number, number][]
      if (!coords || coords.length < 2) return []
-      return [(
+      return [{ place, coords, cased: hasManualTrackColor(place), color: resolveTrackColor(place) }]
+    } catch { return [] }
+  }), [places])
+
+  // Keeps the click handler out of the polyline memo below: `handleMarkerClick`
+  // changes on every selection, and depending on it would redraw all tracks.
+  const markerClickRef = useRef(handleMarkerClick)
+  markerClickRef.current = handleMarkerClick
+
+  const gpxPolylines = useMemo(() => (
+    <>
+      {/* Casings live in their own pane below the lines. Leaflet stacks paths by
+          insertion order, so drawing them inline would put the casing of a track
+          added later — or of one just given a colour — on top of an earlier
+          track's line. Always rendered, hidden via opacity when there is no
+          colour, so toggling one never remounts the path. */}
+      {gpxTracks.map(({ place, coords, cased }) => (
+        <Polyline
+          key={`gpx-${place.id}-casing`}
+          positions={coords}
+          pane={hasCasingPane ? TRACK_CASING_PANE : undefined}
+          pathOptions={{ color: '#ffffff', weight: 6.5, opacity: cased ? 0.7 : 0, lineCap: 'round', lineJoin: 'round' }}
+          interactive={false}
+        />
+      ))}
+      {/* pathOptions, not bare color/weight props: react-leaflet only calls
+          setStyle when the pathOptions reference changes, so bare props would
+          stick at their mount-time colour and a repaint would never arrive. */}
+      {gpxTracks.map(({ place, coords, cased, color }) => (
        <Polyline
          key={`gpx-${place.id}`}
          positions={coords}
-          color={place.category_color || '#3b82f6'}
-          weight={3.5}
-          opacity={0.75}
+          pathOptions={{ color, weight: 3.5, opacity: cased ? 0.9 : 0.75 }}
+          interactive={false}
        />
-      )]
-    } catch { return [] }
-  }), [places])
+      ))}
+      {/* Invisible fat line on top so the track can actually be hit — 3.5px is
+          not a target, least of all on touch, and the start markers cluster below
+          zoom 11. bubblingMouseEvents is essential: paths bubble to the map by
+          default (markers do not), and the map's own click handler clears the
+          selection this one just made. */}
+      {gpxTracks.map(({ place, coords }) => (
+        <Polyline
+          key={`gpx-${place.id}-hit`}
+          positions={coords}
+          pathOptions={{ color: '#000', weight: 14, opacity: 0, lineCap: 'round', lineJoin: 'round' }}
+          bubblingMouseEvents={false}
+          eventHandlers={{ click: () => markerClickRef.current(place.id) }}
+        />
+      ))}
+    </>
+  ), [gpxTracks, hasCasingPane])

  const TooltipOverlay = !hoverDisabled && hoveredPlace && tooltipPos && !isTouchDevice
  const CatIcon = TooltipOverlay ? getCategoryIcon(hoveredPlace.category_icon) : null
@@ -776,6 +845,7 @@ export const MapView = memo(function MapView({
      ] : [])}

      {/* GPX imported route geometries */}
+      <TrackCasingPane onReady={setHasCasingPane} />
      {gpxPolylines}

      <ReservationOverlay
@@ -676,4 +676,38 @@ describe('MapViewGL', () => {

    expect(glMap.fitBounds.mock.calls.length).toBe(afterDayFit)
  })
+// ── Track colours (#776) ──────────────────────────────────────────────────
+  // The gpx layer paints from a per-feature property, so what matters is the
+  // GeoJSON handed to setData. getSource returns null by default in this suite,
+  // which makes the effect bail out — the source has to be stubbed per test.
+  const gpxFeatures = () => {
+    const gpxSource = { setData: vi.fn() }
+    glMap.getSource.mockImplementation((id: string) => (id === 'trip-gpx' ? gpxSource : null))
+    return () => {
+      const calls = gpxSource.setData.mock.calls
+      return calls.length ? (calls[calls.length - 1][0]?.features ?? []) : []
+    }
+  }
+
+  it('FE-COMP-MAPVIEWGL-021: a picked route_color reaches the feature, casing on', async () => {
+    const features = gpxFeatures()
+    const places = [buildMapPlace({
+      id: 5, lat: 48, lng: 2, route_geometry: '[[48.0,2.0],[49.0,3.0]]',
+      category_color: '#00ff00', route_color: '#e11d48',
+    })]
+    render(<MapViewGL places={places} fitKey={1} glProvider="maplibre-gl" />)
+    await act(async () => {})
+    expect(features()[0]?.properties).toMatchObject({ color: '#e11d48', cased: true, place_id: 5 })
+  })
+
+  it('FE-COMP-MAPVIEWGL-022: without a pick it stays on the category colour and skips the casing', async () => {
+    const features = gpxFeatures()
+    const places = [buildMapPlace({
+      id: 6, lat: 48, lng: 2, route_geometry: '[[48.0,2.0],[49.0,3.0]]',
+      category_color: '#00ff00',
+    })]
+    render(<MapViewGL places={places} fitKey={1} glProvider="maplibre-gl" />)
+    await act(async () => {})
+    expect(features()[0]?.properties).toMatchObject({ color: '#00ff00', cased: false })
+  })
 })
@@ -19,6 +19,7 @@ import LocationButton from './LocationButton'
 import { useGeolocation } from '../../hooks/useGeolocation'
 import type { Place, Reservation, RouteVia } from '../../types'
 import { POI_CATEGORY_BY_KEY, type Poi } from './poiCategories'
+import { resolveTrackColor, hasManualTrackColor } from './trackColors'
 import { buildPoiPopupHtml } from './placePopup'
 import { pluginsApi, type PluginMapMarker, type PluginMapLayer } from '../../api/client'
 import { DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults'
@@ -41,6 +42,7 @@ const PLACE_CLUSTER_SOURCE_ID = 'trip-place-clusters'
 const PLACE_CLUSTER_CIRCLE_LAYER_ID = 'trip-place-clusters-circle'
 const PLACE_CLUSTER_COUNT_LAYER_ID = 'trip-place-clusters-count'
 const PLACE_UNCLUSTERED_LAYER_ID = 'trip-place-unclustered-hit'
+const GPX_HIT_LAYER_ID = 'trip-gpx-hit'

 type PlaceWithCoords = Place & { lat: number; lng: number }

@@ -517,6 +519,17 @@ export function MapViewGL({
      // gpx geometries source (place.route_geometry)
      if (!map.getSource('trip-gpx')) {
        map.addSource('trip-gpx', { type: 'geojson', data: { type: 'FeatureCollection', features: [] } })
+        // Casing under the tracks that carry a picked colour (#776) — keeps them
+        // legible on satellite and dark styles. Untouched tracks are filtered
+        // out, so they look exactly as they did before.
+        map.addLayer({
+          id: 'trip-gpx-casing',
+          type: 'line',
+          source: 'trip-gpx',
+          filter: ['==', ['get', 'cased'], true],
+          paint: { 'line-color': '#ffffff', 'line-width': 6.5, 'line-opacity': 0.7 },
+          layout: { 'line-cap': 'round', 'line-join': 'round' },
+        })
        map.addLayer({
          id: 'trip-gpx-line',
          type: 'line',
@@ -524,10 +537,45 @@ export function MapViewGL({
          paint: {
            'line-color': ['coalesce', ['get', 'color'], '#3b82f6'],
            'line-width': 3.5,
-            'line-opacity': 0.75,
+            'line-opacity': ['case', ['==', ['get', 'cased'], true], 0.9, 0.75],
          },
          layout: { 'line-cap': 'round', 'line-join': 'round' },
        })
+        // Invisible fat line that catches the click — 3.5px is not a target,
+        // and the start markers cluster below zoom 11, so without this a track
+        // is unreachable at the very zoom where you compare walks side by side.
+        map.addLayer({
+          id: GPX_HIT_LAYER_ID,
+          type: 'line',
+          source: 'trip-gpx',
+          paint: { 'line-color': '#000', 'line-width': 14, 'line-opacity': 0 },
+          layout: { 'line-cap': 'round', 'line-join': 'round' },
+        })
+        const selectTrack = (e: any) => { // eslint-disable-line @typescript-eslint/no-explicit-any
+          // A click on a cluster bubble sitting over a track belongs to the
+          // cluster (zoom-to-expand), not to the line underneath it.
+          if (
+            typeof map.getLayer === 'function'
+            && map.getLayer(PLACE_CLUSTER_CIRCLE_LAYER_ID)
+            && typeof map.queryRenderedFeatures === 'function'
+            && map.queryRenderedFeatures(e.point, { layers: [PLACE_CLUSTER_CIRCLE_LAYER_ID, PLACE_CLUSTER_COUNT_LAYER_ID] }).length > 0
+          ) return
+          const target = e.originalEvent?.target as HTMLElement | undefined
+          if (target?.closest?.('.mapboxgl-marker, .maplibregl-marker')) return
+          const placeId = e.features?.[0]?.properties?.place_id
+          if (typeof placeId === 'number') onClickRefs.current.marker?.(placeId)
+        }
+        const setTrackCursor = () => {
+          const canvas = typeof map.getCanvas === 'function' ? map.getCanvas() : null
+          if (canvas) canvas.style.cursor = 'pointer'
+        }
+        const clearTrackCursor = () => {
+          const canvas = typeof map.getCanvas === 'function' ? map.getCanvas() : null
+          if (canvas) canvas.style.cursor = ''
+        }
+        map.on('click', GPX_HIT_LAYER_ID, selectTrack)
+        map.on('mouseenter', GPX_HIT_LAYER_ID, setTrackCursor)
+        map.on('mouseleave', GPX_HIT_LAYER_ID, clearTrackCursor)
      }
      if (!map.getSource(PLACE_CLUSTER_SOURCE_ID)) {
        map.addSource(PLACE_CLUSTER_SOURCE_ID, {
@@ -674,6 +722,14 @@ export function MapViewGL({
        && typeof map.queryRenderedFeatures === 'function'
        && map.queryRenderedFeatures(e.point, { layers: [PLACE_CLUSTER_CIRCLE_LAYER_ID, PLACE_CLUSTER_COUNT_LAYER_ID] }).length > 0
      ) return
+      // Same for a click that landed on a track — it selects the track, it does
+      // not drop a new place on top of the line.
+      if (
+        typeof map.getLayer === 'function'
+        && map.getLayer(GPX_HIT_LAYER_ID)
+        && typeof map.queryRenderedFeatures === 'function'
+        && map.queryRenderedFeatures(e.point, { layers: [GPX_HIT_LAYER_ID] }).length > 0
+      ) return
      onClickRefs.current.map?.({ latlng: { lat: e.lngLat.lat, lng: e.lngLat.lng } })
    })
    // Emit the viewport bbox (pan/zoom + once on first idle) so the POI-explore
@@ -1155,7 +1211,11 @@ export function MapViewGL({
        if (!coords || coords.length < 2) return []
        return [{
          type: 'Feature' as const,
-          properties: { color: (place as Place & { category_color?: string }).category_color || '#3b82f6' },
+          properties: {
+            color: resolveTrackColor(place),
+            cased: hasManualTrackColor(place),
+            place_id: place.id,
+          },
          geometry: { type: 'LineString' as const, coordinates: coords.map(([lat, lng]) => [lng, lat]) },
        }]
      } catch { return [] }
@@ -0,0 +1,32 @@
+import { TRACK_COLOR_FALLBACK } from '@trek/shared'
+import type { Place } from '../../types'
+
+/**
+ * The colour a track's polyline is drawn in (#776): a manually picked colour
+ * wins, then the category colour, then the blue every imported track used to
+ * get. Both map renderers and the sidebar chip go through here so a track
+ * never reads as one colour in the list and another on the map.
+ *
+ * `category_color` is a SQL alias joined onto the row, not part of the place
+ * contract, which is why it needs the cast.
+ */
+export function resolveTrackColor(place: Place): string {
+  const withCategory = place as Place & { category_color?: string | null }
+  return place.route_color || withCategory.category_color || TRACK_COLOR_FALLBACK
+}
+
+/**
+ * What the track would fall back to if its colour were cleared — the category
+ * colour, or the old blue. Deliberately ignores route_color: this is what the
+ * picker's auto cell previews, and previewing the current colour there would
+ * promise the opposite of what clicking it does.
+ */
+export function inheritedTrackColor(place: Place): string {
+  const withCategory = place as Place & { category_color?: string | null }
+  return withCategory.category_color || TRACK_COLOR_FALLBACK
+}
+
+/** True once a track carries a deliberately chosen colour rather than an inherited one. */
+export function hasManualTrackColor(place: Place): boolean {
+  return Boolean(place.route_color)
+}
@@ -112,6 +112,7 @@ export function buildPlace(overrides: Partial<Place> = {}): Place {
    google_place_id: null,
    osm_id: null,
    route_geometry: null,
+    route_color: null,
    place_time: null,
    end_time: null,
    duration_minutes: 60,