feat(share): number the day's stops on the map and connect them in order

The public share page drew every geocoded place of the trip as an identical pin with no sequence and no line, so an itinerary could not be read off the map at all (#1962). It does filter to a day, but the only control for that sat below a 300px map, which is why the reporter never found it on a phone. Markers of the selected day now carry their position in that day's order, with the same contract the planner uses: a stop the day visits twice shows both, e.g. "1 · 3". A dashed line connects them in order. It is a straight connector on purpose, not a road route — drawing a real one would mean sending the itinerary to a third-party routing service for every anonymous visitor of a shared link, with no way for the trip's owner to opt out. A day picker now sits directly above the map, driving the same state as the day cards below, so the two can never disagree. Two things nobody reported, both found while in here. Outside a day selection every marker fell back to the placeholder indigo, because the payload nests the category on a day's assignments but sends it flat on the trip-wide pool and only the nested shape was read. And the map refit itself whenever the page re-rendered for an unrelated reason — the language picker, a late FX response — because the effect keyed on the array identity of a list the page rebuilds every render, throwing away the viewer's pan and zoom. It keys on the coordinates now.
2026-10-18 21:16:47 +00:00 · 2026-08-18 17:27:37 +02:00
parent f4ee06e931
commit a3d1fc7808
2 changed files with 254 additions and 14 deletions
@@ -7,6 +7,7 @@ import { resetAllStores, seedStore } from '../../tests/helpers/store';
 import { buildSettings } from '../../tests/helpers/factories';
 import { useSettingsStore } from '../store/settingsStore';
 import SharedTripPage from './SharedTripPage';
+import L from 'leaflet';

 // Mock react-leaflet (SharedTripPage renders a map)
 vi.mock('react-leaflet', () => ({
@@ -15,6 +16,7 @@ vi.mock('react-leaflet', () => ({
  ),
  TileLayer: () => null,
  Marker: ({ children }: { children?: React.ReactNode }) => <div>{children}</div>,
+  Polyline: () => <div data-testid="route-line" />,
  Tooltip: ({ children }: { children?: React.ReactNode }) => <div>{children}</div>,
  useMap: () => ({
    fitBounds: vi.fn(),
@@ -500,8 +502,11 @@ describe('SharedTripPage', () => {
      );
      renderSharedTrip('test-token');
      // The untitled day shows the German label "Tag 1", proving the hardcoded English
-      // "Day 1" was replaced by the i18n key t('dayplan.dayN').
-      await waitFor(() => expect(screen.getByText('Tag 1')).toBeInTheDocument());
+      // "Day 1" was replaced by the i18n key t('dayplan.dayN'). It appears twice since
+      // the day picker above the map was added (#1962): once as a chip, once on the card.
+      await waitFor(() => expect(screen.getAllByText('Tag 1')).toHaveLength(2));
+      const labels = screen.getAllByText('Tag 1');
+      expect(labels.filter((el) => el.closest('button'))).toHaveLength(1);
    });
  });

@@ -701,7 +706,10 @@ describe('SharedTripPage', () => {
        accommodations: [{ id: 3, place_name: 'Hotel Lutetia', start_day_id: 5, end_day_id: 5 }],
      }));

-      expect(screen.getByText('Day 1')).toBeInTheDocument();
+      // Twice since the day picker landed (#1962): the chip above the map and the card.
+      const dayLabels = screen.getAllByText('Day 1');
+      expect(dayLabels).toHaveLength(2);
+      expect(dayLabels.filter((el) => el.closest('button'))).toHaveLength(1);
      expect(screen.getByText('Hotel Lutetia')).toBeInTheDocument();
      expect(screen.getByText('0 places')).toBeInTheDocument();
      // No date row for an undated day.
@@ -785,6 +793,158 @@ describe('SharedTripPage', () => {
    });
  });

+  // ── Day order, route line and the day picker at the map (#1962) ────────────
+  describe('FE-PAGE-SHARED-038: day markers carry their order and a connecting line', () => {
+    const day = { id: 7, trip_id: 1, day_number: 1, date: '2026-07-02', title: 'Day One' };
+    const louvre = { id: 201, name: 'Louvre', lat: 48.86, lng: 2.33, category: { id: 1, name: 'Sight', color: '#ff0000', icon: 'landmark' } };
+    const orsay = { id: 202, name: 'Orsay', lat: 48.85, lng: 2.32, category: null };
+    const notre = { id: 203, name: 'Notre-Dame', lat: 48.853, lng: 2.35, category: null };
+
+    // Every divIcon call the last render produced, as raw html strings.
+    const iconHtml = () => (L.divIcon as unknown as ReturnType<typeof vi.fn>).mock.calls.map((c: any[]) => String(c[0].html));
+
+    beforeEach(() => (L.divIcon as unknown as ReturnType<typeof vi.fn>).mockClear());
+
+    it('numbers the stops by order_index, not by payload order', async () => {
+      await open('order-map-token', payload({
+        days: [day],
+        places: [louvre, orsay, notre],
+        assignments: {
+          '7': [
+            { id: 302, day_id: 7, place_id: 202, order_index: 1, place: orsay },
+            { id: 303, day_id: 7, place_id: 203, order_index: 2, place: notre },
+            { id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre },
+          ],
+        },
+      }));
+
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() => expect(iconHtml().some((h: string) => h.includes('>1<'))).toBe(true));
+
+      const html = iconHtml();
+      // Louvre is order_index 0 and therefore 1, regardless of where it sat in the payload.
+      expect(html.filter((h: string) => h.includes('>1<'))).toHaveLength(1);
+      expect(html.filter((h: string) => h.includes('>2<'))).toHaveLength(1);
+      expect(html.filter((h: string) => h.includes('>3<'))).toHaveLength(1);
+    });
+
+    it('shows both positions once for a stop the day visits twice, and renders it once', async () => {
+      await open('twice-token', payload({
+        days: [day],
+        places: [louvre, orsay],
+        assignments: {
+          '7': [
+            { id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre },
+            { id: 302, day_id: 7, place_id: 202, order_index: 1, place: orsay },
+            { id: 303, day_id: 7, place_id: 201, order_index: 2, place: louvre },
+          ],
+        },
+      }));
+
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() => expect(iconHtml().some((h: string) => h.includes('1 \u00b7 3'))).toBe(true));
+      // One marker for the repeated place, not two with the same React key.
+      expect(iconHtml().filter((h: string) => h.includes('1 \u00b7 3'))).toHaveLength(1);
+    });
+
+    it('leaves the trip-wide pool unnumbered, since it has no order to show', async () => {
+      await open('nonum-token', payload({
+        days: [day],
+        places: [louvre, orsay],
+        assignments: { '7': [{ id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre }] },
+      }));
+
+      await waitFor(() => expect(iconHtml().length).toBeGreaterThan(0));
+      expect(iconHtml().some((h: string) => h.includes('border-radius:8px'))).toBe(false);
+    });
+
+    it('draws the connecting line only for a day with more than one stop', async () => {
+      await open('line-token', payload({
+        days: [day],
+        places: [louvre, orsay],
+        assignments: {
+          '7': [
+            { id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre },
+            { id: 302, day_id: 7, place_id: 202, order_index: 1, place: orsay },
+          ],
+        },
+      }));
+
+      // No day selected: no line, even though the trip has two geocoded places.
+      expect(screen.queryByTestId('route-line')).toBeNull();
+
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() => expect(screen.getByTestId('route-line')).toBeInTheDocument());
+
+      fireEvent.click(screen.getByRole('button', { name: 'All' }));
+      await waitFor(() => expect(screen.queryByTestId('route-line')).toBeNull());
+    });
+
+    it('does not draw a line for a day with a single stop', async () => {
+      await open('single-token', payload({
+        days: [day],
+        places: [louvre],
+        assignments: { '7': [{ id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre }] },
+      }));
+
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() => expect(screen.getAllByText('Louvre').length).toBeGreaterThan(1));
+      expect(screen.queryByTestId('route-line')).toBeNull();
+    });
+  });
+
+  describe('FE-PAGE-SHARED-039: the day picker sits at the map and agrees with the day card', () => {
+    const day = { id: 7, trip_id: 1, day_number: 1, date: '2026-07-02', title: 'Day One' };
+    const louvre = { id: 201, name: 'Louvre', lat: 48.86, lng: 2.33, category: null };
+    const orsay = { id: 202, name: 'Orsay', lat: 48.85, lng: 2.32, category: null };
+
+    it('narrows the markers to one day and back again', async () => {
+      await open('picker-token', payload({
+        days: [day],
+        places: [louvre, orsay],
+        assignments: { '7': [{ id: 301, day_id: 7, place_id: 201, order_index: 0, place: louvre }] },
+      }));
+
+      // All: both geocoded places are on the map, so Orsay's tooltip is present.
+      expect(screen.getByText('Orsay')).toBeInTheDocument();
+
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() => expect(screen.queryByText('Orsay')).toBeNull());
+
+      fireEvent.click(screen.getByRole('button', { name: 'All' }));
+      await waitFor(() => expect(screen.getByText('Orsay')).toBeInTheDocument());
+    });
+
+    it('marks the active chip for assistive tech', async () => {
+      await open('pressed-token', payload({ days: [day], places: [louvre], assignments: {} }));
+
+      expect(screen.getByRole('button', { name: 'All' })).toHaveAttribute('aria-pressed', 'true');
+      fireEvent.click(screen.getByRole('button', { name: 'Day 1' }));
+      await waitFor(() =>
+        expect(screen.getByRole('button', { name: 'Day 1' })).toHaveAttribute('aria-pressed', 'true'),
+      );
+      expect(screen.getByRole('button', { name: 'All' })).toHaveAttribute('aria-pressed', 'false');
+    });
+  });
+
+  describe('FE-PAGE-SHARED-040: trip-wide markers keep their category colour', () => {
+    // The payload nests the category on a day's assignments but sends it flat on the
+    // trip-wide pool, so reading only the nested shape painted every marker indigo.
+    it('reads the flat category_color the trip pool sends', async () => {
+      (L.divIcon as unknown as ReturnType<typeof vi.fn>).mockClear();
+      await open('flatcat-token', payload({
+        days: [],
+        places: [{ id: 201, name: 'Louvre', lat: 48.86, lng: 2.33, category_color: '#ff8800', category_icon: 'landmark' }],
+        assignments: {},
+      }));
+
+      await waitFor(() => expect((L.divIcon as unknown as ReturnType<typeof vi.fn>).mock.calls.length).toBeGreaterThan(0));
+      const html = (L.divIcon as unknown as ReturnType<typeof vi.fn>).mock.calls.map((c: any[]) => String(c[0].html));
+      expect(html.some((h: string) => h.includes('#ff8800'))).toBe(true);
+      expect(html.some((h: string) => h.includes('#6366f1'))).toBe(false);
+    });
+  });
+
  describe('FE-PAGE-SHARED-026: expanded day renders notes and single-leg transports', () => {
    const day = { id: 9, trip_id: 1, day_number: 1, date: '2026-07-02', title: 'Day One' };

@@ -17,7 +17,7 @@ import {
 } from 'lucide-react';
 import { createElement, useEffect, useRef } from 'react';
 import { renderIconMarkup } from '../utils/iconMarkup';
-import { MapContainer, Marker, TileLayer, Tooltip, useMap } from 'react-leaflet';
+import { MapContainer, Marker, Polyline, TileLayer, Tooltip, useMap } from 'react-leaflet';
 import { getCategoryIcon } from '../components/shared/categoryIcons';
 import { DEFAULT_MAP_CENTER, DEFAULT_MAP_ZOOM } from '../constants/mapDefaults';
 import { SUPPORTED_LANGUAGES, useTranslation } from '../i18n';
@@ -33,24 +33,41 @@ import { useSharedTrip } from './sharedTrip/useSharedTrip';

 const TRANSPORT_ICONS = { flight: Plane, train: Train, bus: Bus, car: Car, cruise: Ship };

-function createMarkerIcon(place: any) {
+// Injected into Leaflet's marker HTML, where CSS variables cannot reach - the same
+// reason MapView.tsx is exempt from theme:lint outright.
+const ORDER_BADGE_STYLE = 'position:absolute;bottom:-4px;right:-4px;min-width:16px;height:16px;border-radius:8px;padding:0 3px;background:rgba(255,255,255,0.94);border:1.5px solid rgba(0,0,0,0.15);box-shadow:0 1px 4px rgba(0,0,0,0.18);display:flex;align-items:center;justify-content:center;font-weight:800;color:#111827;line-height:1;box-sizing:border-box;white-space:nowrap;'; // theme-lint-disable
+
+function createMarkerIcon(place: any, orderNumbers?: number[] | null) {
  const cat = place.category;
  // This page answers without a guard, so an unescaped colour here reaches
  // people who have no account on the instance at all.
-  const color = safeHexColor(cat?.color, '#6366f1');
-  const CatIcon = getCategoryIcon(cat?.icon);
+  // The payload carries the category in two shapes: nested on a day's assignments,
+  // flat on the trip-wide pool. Reading only the nested one dropped every marker
+  // outside a day selection to the placeholder colour.
+  const color = safeHexColor(cat?.color ?? place.category_color, '#6366f1');
+  const CatIcon = getCategoryIcon(cat?.icon ?? place.category_icon);
  const iconSvg = renderIconMarkup(createElement(CatIcon, { size: 14, strokeWidth: 2, color: 'white' }));
+  // Position in the day's order, same contract as the planner: a stop the day
+  // visits twice shows both, e.g. "1 . 3". The values are array indices, never payload.
+  const badge = orderNumbers?.length
+    ? `<span style="${ORDER_BADGE_STYLE}font-size:${orderNumbers.length > 1 ? 7.5 : 9}px;">${orderNumbers.join(' \u00b7 ')}</span>`
+    : '';
  return L.divIcon({
    className: '',
    iconSize: [28, 28],
    iconAnchor: [14, 14],
-    html: `<div style="width:28px;height:28px;border-radius:50%;background:${color};display:flex;align-items:center;justify-content:center;box-shadow:0 2px 6px rgba(0,0,0,0.3);border:2px solid white;">${iconSvg}</div>`,
+    html: `<div style="position:relative;width:28px;height:28px;border-radius:50%;background:${color};display:flex;align-items:center;justify-content:center;box-shadow:0 2px 6px rgba(0,0,0,0.3);border:2px solid white;">${iconSvg}${badge}</div>`,
  });
 }

 function FitBoundsToPlaces({ places, framedOnMount }: { places: any[]; framedOnMount: boolean }) {
  const map = useMap();
  const fitRan = useRef(false);
+  // The page rebuilds this array on every render (a Page body may not memoise), so
+  // keying the effect on the coordinates rather than the array identity is what stops
+  // an unrelated re-render - the language picker, a late FX response - from throwing
+  // the viewer's pan and zoom away.
+  const fitKey = places.map((p) => `${p.lat},${p.lng}`).join('|');
  useEffect(() => {
    if (places.length === 0) return;
    // The map already opened framed on these places; fitting again would only re-do it.
@@ -62,7 +79,7 @@ function FitBoundsToPlaces({ places, framedOnMount }: { places: any[]; framedOnM
    fitRan.current = true;
    const bounds = L.latLngBounds(places.map((p) => [p.lat, p.lng]));
    map.fitBounds(bounds, { padding: [40, 40], maxZoom: 14 });
-  }, [places, map]); // eslint-disable-line react-hooks/exhaustive-deps
+  }, [fitKey, map]); // eslint-disable-line react-hooks/exhaustive-deps
  return null;
 }

@@ -136,10 +153,27 @@ export default function SharedTripPage() {
  } = data;
  const sortedDays = [...(days || [])].sort((a: any, b: any) => a.day_number - b.day_number);

-  // Map places
-  const mapPlaces = selectedDay
-    ? (assignments[String(selectedDay)] || []).map((a: any) => a.place).filter((p: any) => p?.lat && p?.lng)
-    : (places || []).filter((p: any) => p?.lat && p?.lng);
+  // Map places. In day mode each stop carries its position in the day's order; the
+  // trip-wide pool has none (it arrives by created_at). The index runs over the full
+  // sorted assignment list, like the planner does, so a stop without coordinates still
+  // consumes a number and the app and the share link agree on what "3" means.
+  const dayAssignments = selectedDay
+    ? [...(assignments[String(selectedDay)] || [])].sort((a: any, b: any) => a.order_index - b.order_index)
+    : [];
+  const dayOrderMap: Record<number, number[]> = {};
+  dayAssignments.forEach((a: any, i: number) => {
+    if (!a.place?.id) return;
+    (dayOrderMap[a.place.id] ||= []).push(i + 1);
+  });
+  const dayPlaces: any[] = [];
+  const seenPlaceIds = new Set<number>();
+  for (const a of dayAssignments as any[]) {
+    const p = a.place;
+    if (!p?.lat || !p?.lng || seenPlaceIds.has(p.id)) continue;
+    seenPlaceIds.add(p.id);
+    dayPlaces.push(p);
+  }
+  const mapPlaces = selectedDay ? dayPlaces : (places || []).filter((p: any) => p?.lat && p?.lng);

  // Open framed on the trip's places instead of on Paris. MapContainer only reads center/zoom
  // at mount, so recomputing this per render is free — and the fit below takes over from there.
@@ -396,6 +430,41 @@ export default function SharedTripPage() {
        {/* Map */}
        {activeTab === 'plan' && (
          <>
+            {/* Day picker at the map. Same setter as the day card below, so the map and
+                the expanded day can never disagree. Without it the only way to narrow the
+                map down was a control 300px further down the page. */}
+            {sortedDays.length > 0 && (
+              <div style={{ display: 'flex', gap: 6, overflowX: 'auto', marginBottom: 10, padding: '2px 0' }}>
+                {[null, ...sortedDays.map((d: any) => d.id)].map((id: number | null, i: number) => {
+                  const active = selectedDay === id;
+                  return (
+                    <button
+                      key={id ?? 'all'}
+                      type="button"
+                      onClick={() => setSelectedDay(id)}
+                      aria-pressed={active}
+                      // Same literals as the day-number circle below. This page pins itself
+                      // to the light neutral look (applyAppearance skips /shared/*), so a
+                      // token here would not be value-equal to the rest of the card.
+                      className={active ? 'bg-[#111827] text-white' : 'bg-[#f3f4f6] text-[#6b7280]'} // theme-lint-disable
+                      style={{
+                        padding: '5px 12px',
+                        borderRadius: 999,
+                        border: 'none',
+                        cursor: 'pointer',
+                        whiteSpace: 'nowrap',
+                        fontWeight: 600,
+                        fontFamily: 'inherit',
+                        flexShrink: 0,
+                        fontSize: 'calc(12px * var(--fs-scale-body, 1))',
+                      }}
+                    >
+                      {id === null ? t('day.allDays') : t('dayplan.dayN', { n: sortedDays[i - 1].day_number })}
+                    </button>
+                  );
+                })}
+              </div>
+            )}
            <div
              style={{
                borderRadius: 16,
@@ -416,8 +485,19 @@ export default function SharedTripPage() {
                  referrerPolicy="strict-origin-when-cross-origin"
                />
                <FitBoundsToPlaces places={mapPlaces} framedOnMount={framed !== null} />
+                {selectedDay && mapPlaces.length > 1 && (
+                  <Polyline
+                    positions={mapPlaces.map((p: any) => [p.lat, p.lng])}
+                    // Dashed and straight on purpose: it shows the order of the day's stops,
+                    // not the roads between them. A real route would mean sending the
+                    // itinerary to a third party for every anonymous visitor of a shared
+                    // link, with no way for the trip's owner to opt out.
+                    pathOptions={{ color: '#0a84ff', weight: 3, opacity: 0.8, dashArray: '6 8', lineCap: 'round' }} // theme-lint-disable
+                    interactive={false}
+                  />
+                )}
                {mapPlaces.map((p: any) => (
-                  <Marker key={p.id} position={[p.lat, p.lng]} icon={createMarkerIcon(p)}>
+                  <Marker key={p.id} position={[p.lat, p.lng]} icon={createMarkerIcon(p, dayOrderMap[p.id] ?? null)}>
                    <Tooltip>{p.name}</Tooltip>
                  </Marker>
                ))}