From 4d6820a4d42793cc27fdf3852712801da0e990dd Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 27 Aug 2026 18:56:54 +0200 Subject: [PATCH] @ fix(planner): route a layover flight per leg, and count linked places as planned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two v4.0.0 reports, both a surface answering a question with less than it had. The map built its own waypoint order. It read the BOOKING position and never expanded metadata.legs, while a multi-leg booking stores its position per leg — so a Frankfurt to Vancouver flight via Reykjavik was dropped from the list and the airport before it was joined to the airport after it in one driving run across the Atlantic. It now takes the order from getMergedItems, the same one the day plan renders, which brings the span filter and the leg expansion with it. getMergedItems in turn had never read day_plan_position, the one slot the map did read, which is how the two could disagree at all. getTransportRouteEndpoints handed back the booking outermost airports for every leg. A stop between two legs was therefore routed from the arrival airport back to the departure one — a drive from Vancouver to Frankfurt with a cafe in Reykjavik on the way. A leg now spans its own pair, when the endpoints line up one per stop with the legs; anything ambiguous keeps the role lookup. Being planned meant one thing: a day_assignments row. A hotel is linked through its stay and a venue through its booking, and neither is ever dragged onto a day, so both sat under Unplanned while the day header printed their names. The three places that asked the question — desktop sidebar, map markers, phone browser — now ask one helper that also counts a stay and a day-anchored booking. A booking that names no day still counts as unplanned, because nothing says when it happens, and the day plan renders nothing for it either. @ --- .../Planner/usePlacesSidebar.test.tsx | 35 +++++- .../components/Planner/usePlacesSidebar.ts | 17 ++- client/src/hooks/useRouteCalculation.ts | 69 ++++++------ .../screens/trip/places/MPlacesBrowser.tsx | 7 +- .../trip/places/placesBrowserModel.test.ts | 12 ++ .../screens/trip/places/placesBrowserModel.ts | 18 +-- client/src/pages/TripPlannerPage.tsx | 3 +- .../src/pages/tripPlanner/useTripPlanner.ts | 10 +- client/src/utils/dayMerge.ts | 32 +++++- client/src/utils/plannedPlaces.test.ts | 103 ++++++++++++++++++ client/src/utils/plannedPlaces.ts | 88 +++++++++++++++ .../hooks/useRouteCalculation.test.ts | 88 +++++++++++++++ 12 files changed, 423 insertions(+), 59 deletions(-) create mode 100644 client/src/utils/plannedPlaces.test.ts create mode 100644 client/src/utils/plannedPlaces.ts diff --git a/client/src/components/Planner/usePlacesSidebar.test.tsx b/client/src/components/Planner/usePlacesSidebar.test.tsx index 03918897b..3bf66d52f 100644 --- a/client/src/components/Planner/usePlacesSidebar.test.tsx +++ b/client/src/components/Planner/usePlacesSidebar.test.tsx @@ -1,4 +1,4 @@ -// FE-PLANNER-PSHOOK-001 to FE-PLANNER-PSHOOK-048 +// FE-PLANNER-PSHOOK-001 to FE-PLANNER-PSHOOK-051 import { http, HttpResponse } from 'msw'; import userEvent from '@testing-library/user-event'; import { render, screen, fireEvent, act, waitFor } from '../../../tests/helpers/render'; @@ -137,6 +137,39 @@ describe('usePlacesSidebar filtering', () => { expect([...S.plannedIds]).toEqual([1]); }); + // #2072 — a hotel is linked through its stay, never dragged onto a day, so the + // pool called it unplanned while the day header printed its name. + it('FE-PLANNER-PSHOOK-049: a place linked to a stay counts as planned', () => { + const hotel = buildPlace({ id: 1, name: 'Hotel' }); + const loose = buildPlace({ id: 2, name: 'Loose' }); + const accommodations = [{ place_id: 1, start_day_id: 3, end_day_id: 5 }]; + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Loose']); + act(() => { S.setFilter('planned'); }); + expect(names()).toEqual(['Hotel']); + }); + + it('FE-PLANNER-PSHOOK-050: a place on a day-anchored booking counts as planned', () => { + const venue = buildPlace({ id: 1, name: 'Venue' }); + const loose = buildPlace({ id: 2, name: 'Loose' }); + seedStore(useTripStore, { reservations: [{ id: 9, place_id: 1, day_id: 3 }] }); + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Loose']); + }); + + it('FE-PLANNER-PSHOOK-051: a booking with no day leaves its place unplanned', () => { + const venue = buildPlace({ id: 1, name: 'Venue' }); + seedStore(useTripStore, { reservations: [{ id: 9, place_id: 1, day_id: null }] }); + render(); + + act(() => { S.setFilter('unplanned'); }); + expect(names()).toEqual(['Venue']); + }); + it('FE-PLANNER-PSHOOK-006: the planned filter keeps only assigned places', () => { const planned = buildPlace({ id: 1, name: 'Planned' }); const loose = buildPlace({ id: 2, name: 'Loose' }); diff --git a/client/src/components/Planner/usePlacesSidebar.ts b/client/src/components/Planner/usePlacesSidebar.ts index aab094a62..2b6bd7734 100644 --- a/client/src/components/Planner/usePlacesSidebar.ts +++ b/client/src/components/Planner/usePlacesSidebar.ts @@ -15,10 +15,16 @@ import { placeToSaveTarget } from '../Collections/saveTarget' import type { Place, Category, Day, AssignmentsMap } from '../../types' import { getGoogleMapsUrlForPlace } from './placeGoogleMaps' import { safeHttpUrl } from '../../utils/safeUrl' +import { plannedPlaceIds, type PlannedAccommodation } from '../../utils/plannedPlaces' + +/** Stable identity — a fresh [] default would invalidate the planned memo on every render. */ +const NO_ACCOMMODATIONS: PlannedAccommodation[] = [] export interface PlacesSidebarProps { tripId: number places: Place[] + /** The trip's stays — hook-local state in useTripPlanner, so it arrives as a prop. */ + accommodations?: PlannedAccommodation[] categories: Category[] assignments: AssignmentsMap selectedDayId: number | null @@ -45,13 +51,15 @@ export interface PlacesSidebarProps { */ export function usePlacesSidebar(props: PlacesSidebarProps) { const { - tripId, places, assignments, selectedDayId, + tripId, places, assignments, selectedDayId, accommodations = NO_ACCOMMODATIONS, pushUndo, initialScrollTop, onScrollTopChange, } = props const { t } = useTranslation() const toast = useToast() const ctxMenu = useContextMenu() const trip = useTripStore((s) => s.trip) + // A booking plans the place it points at, so the pool has to see them (#2072). + const reservations = useTripStore((s) => s.reservations) const loadTrip = useTripStore((s) => s.loadTrip) const can = useCanDo() const canEditPlaces = can('place_edit', trip) @@ -216,9 +224,10 @@ export function usePlacesSidebar(props: PlacesSidebarProps) { const hasTracks = useMemo(() => places.some(p => p.route_geometry), [places]) useEffect(() => { if (filter === 'tracks' && !hasTracks) setFilter('all') }, [hasTracks, filter]) - const plannedIds = useMemo(() => new Set( - Object.values(assignments).flatMap(da => da.map(a => a.place?.id).filter(Boolean)) - ), [assignments]) + const plannedIds = useMemo( + () => plannedPlaceIds({ assignments, accommodations, reservations }), + [assignments, accommodations, reservations], + ) const filtered = useMemo(() => { const list = places.filter(p => { diff --git a/client/src/hooks/useRouteCalculation.ts b/client/src/hooks/useRouteCalculation.ts index f66df2349..67de7a8eb 100644 --- a/client/src/hooks/useRouteCalculation.ts +++ b/client/src/hooks/useRouteCalculation.ts @@ -2,7 +2,7 @@ import { useState, useCallback, useRef, useEffect, useMemo } from 'react' import { useTripStore } from '../store/tripStore' import { useSettingsStore } from '../store/settingsStore' import { calculateRouteWithLegs, withHotelBookends, type RouteProfileKey } from '../components/Map/RouteCalculator' -import { getTransportRouteEndpoints } from '../utils/dayMerge' +import { getTransportRouteEndpoints, getTransportForDay, getMergedItems } from '../utils/dayMerge' import { getDayBookendHotels, shouldDrawMorningLeg, shouldDrawEveningLeg } from '../utils/dayOrder' import { resolveLegMode } from '../components/Planner/legMode' import type { TripStoreState } from '../store/tripStore' @@ -54,46 +54,45 @@ export function useRouteCalculation(tripStore: TripStoreState, selectedDayId: nu } const thisOrder = dayOrder(dayId) - // Transport reservations for this day with a known position — mirrors getTransportForDay semantics - const dayTransports = thisOrder == null ? [] : allReservations.filter(r => { - if (!TRANSPORT_TYPES.includes(r.type)) return false - const startId = r.day_id - if (startId == null) return false - const endId = r.end_day_id ?? startId - if (startId === endId) { - if (startId !== dayId) return false - } else { - const startOrder = dayOrder(startId) - const endOrder = dayOrder(endId) - if (startOrder == null || endOrder == null) return false - if (thisOrder < startOrder || thisOrder > endOrder) return false - } - const pos = r.day_positions?.[dayId] ?? r.day_positions?.[String(dayId)] ?? r.day_plan_position - return pos != null + // The order the day plan shows is the order the map has to draw, so take it from + // the same place the plan does rather than rebuilding it here. The old builder + // read the BOOKING's position and never expanded metadata.legs, while a + // multi-leg booking stores its position per leg — so a layover flight was + // dropped from the waypoint list entirely and the airport before it was joined + // to the airport after it in one road run across an ocean (#2071). + // + // getTransportForDay brings the span filter, the hotel/assignment exclusions and + // the leg expansion with it; getMergedItems places each leg by its own saved + // position, falling back to its time. Notes carry no coordinates, so none are + // passed. + const dayTransports = thisOrder == null ? [] : getTransportForDay({ + reservations: allReservations.filter(r => TRANSPORT_TYPES.includes(r.type)), + dayId, + dayAssignmentIds: da.map(a => a.id), + days: allDays, }) + const merged = getMergedItems({ dayAssignments: da, dayNotes: [], dayTransports, dayId }) // Build a unified list of places + transports sorted by effective position. type Entry = | { kind: 'place'; lat: number; lng: number; pos: number; time: string | null; mode: string | null; incoming: string | null } | { kind: 'transport'; from: { lat: number; lng: number } | null; to: { lat: number; lng: number } | null; pos: number } - const entries: Entry[] = [ - ...da.filter(a => a.place?.lat && a.place?.lng).map(a => ({ - kind: 'place' as const, lat: a.place.lat!, lng: a.place.lng!, pos: a.order_index, time: a.place?.place_time ?? null, - // Per-segment travel mode (#1281): mode of the leg leaving this place. - mode: (a as { leg_transport_mode?: string | null }).leg_transport_mode ?? null, - // Boundary-leg mode (#1281 follow-up): mode of the leg arriving at this place. - incoming: (a as { incoming_leg_transport_mode?: string | null }).incoming_leg_transport_mode ?? null, - })), - ...dayTransports.map(r => { - const { from, to } = getTransportRouteEndpoints(r, dayId) - return { - kind: 'transport' as const, - from, - to, - pos: (r.day_positions?.[dayId] ?? r.day_positions?.[String(dayId)] ?? r.day_plan_position) as number, - } - }), - ].sort((a, b) => a.pos - b.pos) + const entries: Entry[] = merged.flatMap((item): Entry[] => { + if (item.type === 'place') { + const a = item.data + if (!a.place?.lat || !a.place?.lng) return [] + return [{ + kind: 'place', lat: a.place.lat, lng: a.place.lng, pos: item.sortKey, time: a.place?.place_time ?? null, + // Per-segment travel mode (#1281): mode of the leg leaving this place. + mode: (a as { leg_transport_mode?: string | null }).leg_transport_mode ?? null, + // Boundary-leg mode (#1281 follow-up): mode of the leg arriving at this place. + incoming: (a as { incoming_leg_transport_mode?: string | null }).incoming_leg_transport_mode ?? null, + }] + } + if (item.type !== 'transport') return [] + const { from, to } = getTransportRouteEndpoints(item.data, dayId) + return [{ kind: 'transport', from, to, pos: item.sortKey }] + }) // Group located places into driving runs. // - A transport WITH a location anchors the route to its departure point (you diff --git a/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx b/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx index f757716df..46b9557df 100644 --- a/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx +++ b/client/src/mobile/screens/trip/places/MPlacesBrowser.tsx @@ -59,7 +59,12 @@ export default function MPlacesBrowser({ planner, shell }: MPlacesBrowserProps) if (filter === 'tracks' && !hasTracks) setFilter('all') }, [filter, hasTracks, setFilter]) - const plannedIds = useMemo(() => plannedPlaceIds(assignments), [assignments]) + // A hotel is linked through its stay and a venue through its booking; neither is + // ever dragged onto a day, and the pool used to call both unplanned (#2072). + const plannedIds = useMemo( + () => plannedPlaceIds(assignments, planner.tripAccommodations, planner.reservations), + [assignments, planner.tripAccommodations, planner.reservations], + ) const dayNumberByPlace = useMemo(() => firstPlannedDayNumbers(assignments, days), [assignments, days]) const filtered = useMemo( () => filterPool(places, { filter, categoryFilters, search, plannedIds }), diff --git a/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts b/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts index f1f18b527..d26d9a9b6 100644 --- a/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts +++ b/client/src/mobile/screens/trip/places/placesBrowserModel.test.ts @@ -151,4 +151,16 @@ describe('placesBrowserModel — tracks pool', () => { expect(c.planned).toBe(1) expect(c.unplanned).toBe(1) }) + + // #2072 — the phone pool has to agree with the desktop one; both ask the same + // shared helper now. + it('FE-MOB-PMODEL-014: a stay and a day-anchored booking both count as planned', () => { + const ids = plannedPlaceIds( + {}, + [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + [{ place_id: 8, day_id: 10 }, { place_id: 9, day_id: null }], + ) + expect([...ids].sort((a, b) => a - b)).toEqual([7, 8]) + }) + }) diff --git a/client/src/mobile/screens/trip/places/placesBrowserModel.ts b/client/src/mobile/screens/trip/places/placesBrowserModel.ts index 1e62c9a76..fae376d1e 100644 --- a/client/src/mobile/screens/trip/places/placesBrowserModel.ts +++ b/client/src/mobile/screens/trip/places/placesBrowserModel.ts @@ -1,4 +1,8 @@ import type { AssignmentsMap, Day, Place } from '../../../../types' +import { + plannedPlaceIds as sharedPlannedPlaceIds, + type PlannedAccommodation, type PlannedReservation, +} from '../../../../utils/plannedPlaces' /** * Pure filter model of the mobile places browser. Mirrors the desktop @@ -7,14 +11,12 @@ import type { AssignmentsMap, Day, Place } from '../../../../types' * and the map markers always agree (#1541). */ -export function plannedPlaceIds(assignments: AssignmentsMap): Set { - const ids = new Set() - for (const dayAssignments of Object.values(assignments)) { - for (const a of dayAssignments) { - if (a.place?.id != null) ids.add(a.place.id) - } - } - return ids +export function plannedPlaceIds( + assignments: AssignmentsMap, + accommodations: PlannedAccommodation[] = [], + reservations: PlannedReservation[] = [], +): Set { + return sharedPlannedPlaceIds({ assignments, accommodations, reservations }) } /** place id → number of the first (lowest-numbered) day it is planned on. */ diff --git a/client/src/pages/TripPlannerPage.tsx b/client/src/pages/TripPlannerPage.tsx index 6f7c99375..ff0a0ea4d 100644 --- a/client/src/pages/TripPlannerPage.tsx +++ b/client/src/pages/TripPlannerPage.tsx @@ -553,6 +553,7 @@ function TripPlannerPageDesktop(): React.ReactElement | null { places={places} categories={categories} assignments={assignments} + accommodations={tripAccommodations} selectedDayId={selectedDayId} selectedPlaceId={selectedPlaceId} onPlaceClick={handlePlaceClick} @@ -711,7 +712,7 @@ function TripPlannerPageDesktop(): React.ReactElement | null {
{mobileSidebarOpen === 'left' ? { handleSelectDay(id); setMobileSidebarOpen(null) }} onPlaceClick={(placeId, assignmentId) => { handlePlaceClick(placeId, assignmentId) }} onReorder={handleReorder} onReorderDays={handleReorderDays} onAddDay={handleAddDay} onUpdateDayTitle={handleUpdateDayTitle} onAssignToDay={handleAssignToDay} onRouteCalculated={(r) => { if (r) { setRoute([r.coordinates]); setRouteInfo(r) } else { setRoute(null); setRouteInfo(null) } }} reservations={reservations} visibleConnectionIds={visibleConnections} onToggleConnection={toggleConnection} allConnectionsShown={allConnectionsShown} onToggleAllConnections={toggleAllConnections} onAddReservation={(dayId) => { setEditingReservation(null); tripActions.setSelectedDay(dayId); setShowReservationModal(true); setMobileSidebarOpen(null) }} onAddTransport={can('day_edit', trip) ? (dayId) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill(null); setTransportModalAutomated(false); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onOpenTransit={(r) => { setTransitJourney(r); setMobileSidebarOpen(null) }} onPlanTransit={can('day_edit', trip) && tripHasDates ? (dayId) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill(null); setTransportModalAutomated(true); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onPlanTransitLeg={can('day_edit', trip) && tripHasDates ? ({ dayId, from, to, time }) => { setTransportModalDayId(dayId); setEditingTransport(null); setTransitPrefill({ from, to, time }); setTransportModalAutomated(true); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onDayDetail={(day) => { setShowDayDetail(day); setSelectedPlaceId(null); selectAssignment(null) }} onRemoveAssignment={handleRemoveAssignment} onEditPlace={(place, assignmentId) => { setEditingPlace(place); setEditingAssignmentId(assignmentId || null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} accommodations={tripAccommodations} routeShown={routeShown} routeProfile={routeProfile} onToggleRoute={() => setRouteShown(v => !v)} onSetRouteProfile={setRouteProfile} onNavigateToFiles={() => { setMobileSidebarOpen(null); handleTabChange('dateien') }} onExpandedDaysChange={setExpandedDayIds} pushUndo={pushUndo} canUndo={canUndo} lastActionLabel={lastActionLabel} onUndo={handleUndo} onEditTransport={can('day_edit', trip) ? (reservation) => { setEditingTransport(reservation); setTransportModalDayId(reservation.day_id ?? null); setShowTransportModal(true); setMobileSidebarOpen(null) } : undefined} onEditReservation={can('reservation_edit', trip) ? (r) => { setEditingReservation(r); setShowReservationModal(true); setMobileSidebarOpen(null) } : undefined} initialScrollTop={mobilePlanScrollTopRef.current} onScrollTopChange={(top) => { mobilePlanScrollTopRef.current = top }} showRouteToolsWhenExpanded isMobile /> - : { handlePlaceClick(placeId); setMobileSidebarOpen(null) }} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onAssignToDay={handleAssignToDay} onEditPlace={(place) => { openPlaceEditor(place); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} onBulkDeletePlaces={(ids) => setDeletePlaceIds(ids)} onBulkDeleteConfirm={(ids) => confirmDeletePlaces(ids)} onBulkChangeCategory={(ids, catId) => confirmChangeCategory(ids, catId)} days={days} isMobile pushUndo={pushUndo} initialScrollTop={mobilePlacesScrollTopRef.current} onScrollTopChange={(top) => { mobilePlacesScrollTopRef.current = top }} /> + : { handlePlaceClick(placeId); setMobileSidebarOpen(null) }} onAddPlace={() => { setEditingPlace(null); setShowPlaceForm(true); setMobileSidebarOpen(null) }} onAssignToDay={handleAssignToDay} onEditPlace={(place) => { openPlaceEditor(place); setMobileSidebarOpen(null) }} onDeletePlace={(placeId) => handleDeletePlace(placeId)} onBulkDeletePlaces={(ids) => setDeletePlaceIds(ids)} onBulkDeleteConfirm={(ids) => confirmDeletePlaces(ids)} onBulkChangeCategory={(ids, catId) => confirmChangeCategory(ids, catId)} days={days} isMobile pushUndo={pushUndo} initialScrollTop={mobilePlacesScrollTopRef.current} onScrollTopChange={(top) => { mobilePlacesScrollTopRef.current = top }} /> }
diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 93172a5a1..5997543d5 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -35,6 +35,7 @@ import { toggleConnectionId, toggleAllConnections as flipAllConnectionsMode, type StoredConnections, } from '../../utils/connectionsVisibility' +import { plannedPlaceIds, plannedPlaceIdsForDay } from '../../utils/plannedPlaces' /** * Trip planner page logic — the big one. Owns the trip store wiring, addon @@ -445,10 +446,9 @@ export function useTripPlanner() { // the whole plan (#2024). 'unplanned' always uses the whole-trip set — a place // assigned to any day is not unplanned. const plannedIds = placesFilter === 'unplanned' || placesFilter === 'planned' - ? new Set((placesFilter === 'planned' && selectedDayId - ? (assignments[String(selectedDayId)] || []) - : Object.values(assignments).flat() - ).map(a => a.place?.id).filter(Boolean)) + ? (placesFilter === 'planned' && selectedDayId + ? plannedPlaceIdsForDay(selectedDayId, days, { assignments, accommodations: tripAccommodations, reservations }) + : plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations })) : null return places.filter(p => { @@ -467,7 +467,7 @@ export function useTripPlanner() { if (placesFilter === 'planned' && plannedIds && !plannedIds.has(p.id)) return false return true }) - }, [places, placesCategoryFilter, placesFilter, assignments, expandedDayIds, selectedDayId]) + }, [places, placesCategoryFilter, placesFilter, assignments, expandedDayIds, selectedDayId, days, tripAccommodations, reservations]) const { route, routeSegments, routeVias, routeInfo, setRoute, setRouteInfo, updateRouteForDay } = useRouteCalculation({ assignments } as any, selectedDayId, routeShown, routeProfile, tripAccommodations) diff --git a/client/src/utils/dayMerge.ts b/client/src/utils/dayMerge.ts index 302925694..d8b5dbb5a 100644 --- a/client/src/utils/dayMerge.ts +++ b/client/src/utils/dayMerge.ts @@ -1,3 +1,8 @@ +// `orderedEndpoints` is the geometry order's single source of truth, in the module +// that documents the multi-leg model. Sorting endpoints a second time here is how +// the two drift. +import { orderedEndpoints } from './flightLegs' + export const TRANSPORT_TYPES = new Set(['flight', 'train', 'bus', 'car', 'taxi', 'bicycle', 'cruise', 'ferry', 'transit', 'transport_other']) export interface MergedItem { @@ -60,9 +65,23 @@ export function getTransportRouteEndpoints( r: any, dayId: number ): { from: { lat: number; lng: number } | null; to: { lat: number; lng: number } | null } { + // A leg of a multi-leg booking spans its OWN pair of airports, endpoints[i] and + // endpoints[i+1] — not the booking's outermost two. Handing back the booking's + // from/to for every leg put a stop between two legs on a road route from the + // arrival airport back to the departure one (#2071). + // + // Only when the endpoints line up one-per-stop with the legs. An endpoint that + // failed to geocode never reaches the row, and a short list would silently map + // every earlier leg to the wrong pair — so an ambiguous booking keeps the role + // lookup it has always used. + const ordered = orderedEndpoints(r) + const legIdx: number | null = + r.__leg && ordered.length === r.__leg.total + 1 ? (r.__leg.index as number) : null + const point = (e: { lat?: number | null; lng?: number | null } | undefined): { lat: number; lng: number } | null => + e && e.lat != null && e.lng != null ? { lat: e.lat, lng: e.lng } : null const ep = (role: 'from' | 'to'): { lat: number; lng: number } | null => { - const e = (r.endpoints || []).find((x: any) => x.role === role) - return e && e.lat != null && e.lng != null ? { lat: e.lat, lng: e.lng } : null + if (legIdx != null) return point(ordered[role === 'from' ? legIdx : legIdx + 1]) + return point((r.endpoints || []).find((x: { role?: string }) => x.role === role)) } switch (getSpanPhase(r, dayId)) { case 'start': @@ -240,8 +259,13 @@ export function getMergedItems(opts: { const timed = timedTransports[ti] const minutes = timed.minutes - // Per-day position takes precedence (set by user reorder) - const perDayPos = timed.data.day_positions?.[dayId] ?? timed.data.day_positions?.[String(dayId)] + // Per-day position takes precedence (set by user reorder), then the booking's + // own slot. The map's waypoint builder has always read day_plan_position and + // this never did, so a booking carrying only that value sat between the places + // on the map and at the bottom of the day everywhere else (#2071). + const perDayPos = timed.data.day_positions?.[dayId] + ?? timed.data.day_positions?.[String(dayId)] + ?? timed.data.day_plan_position if (perDayPos != null) { result.push({ type: timed.type, sortKey: perDayPos, data: timed.data }) continue diff --git a/client/src/utils/plannedPlaces.test.ts b/client/src/utils/plannedPlaces.test.ts new file mode 100644 index 000000000..a0c786868 --- /dev/null +++ b/client/src/utils/plannedPlaces.test.ts @@ -0,0 +1,103 @@ +import { describe, it, expect } from 'vitest' +import { plannedPlaceIds, plannedPlaceIdsForDay } from './plannedPlaces' +import type { AssignmentsMap, Day } from '../types' + +// FE-UTIL-PLANNED-001 to FE-UTIL-PLANNED-010 +// +// #2072 — a hotel linked to a booking sat under "Unplanned" while its name was +// printed in the day header, because being planned meant one thing only: a +// day_assignments row. + +const assignments = (map: Record): AssignmentsMap => + Object.fromEntries( + Object.entries(map).map(([dayId, ids]) => [dayId, ids.map((id, i) => ({ + id: 900 + id, day_id: Number(dayId), order_index: i, place: { id }, + }))]), + ) as unknown as AssignmentsMap + +const DAYS = [ + { id: 10, day_number: 1 }, { id: 11, day_number: 2 }, { id: 12, day_number: 3 }, { id: 13, day_number: 4 }, +] as unknown as Day[] + +describe('plannedPlaceIds', () => { + it('FE-UTIL-PLANNED-001: an assigned place is planned, as it always was', () => { + expect([...plannedPlaceIds({ assignments: assignments({ 10: [1, 2] }) })]).toEqual([1, 2]) + }) + + it('FE-UTIL-PLANNED-002: a stay plans its place without any assignment', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + }) + expect(ids.has(7)).toBe(true) + }) + + it('FE-UTIL-PLANNED-003: a stay whose place was deleted contributes nothing', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + accommodations: [{ place_id: null, start_day_id: 10, end_day_id: 12 }], + }) + expect(ids.size).toBe(0) + }) + + it('FE-UTIL-PLANNED-004: a booking on a day plans the place it points at', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + reservations: [{ place_id: 5, day_id: 10 }], + }) + expect(ids.has(5)).toBe(true) + }) + + // The boundary the issue itself calls out: a booking that names no day is not + // on the plan, and the day view renders nothing for it either. + it('FE-UTIL-PLANNED-005: a booking with no day plans nothing', () => { + const ids = plannedPlaceIds({ + assignments: assignments({}), + reservations: [{ place_id: 5, day_id: null }], + }) + expect(ids.size).toBe(0) + }) + + it('FE-UTIL-PLANNED-006: a place reachable two ways is counted once', () => { + const ids = plannedPlaceIds({ + assignments: assignments({ 10: [7] }), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + reservations: [{ place_id: 7, day_id: 10 }], + }) + expect([...ids]).toEqual([7]) + }) + + it('FE-UTIL-PLANNED-007: no sources at all is an empty set, not a throw', () => { + expect(plannedPlaceIds({ assignments: {} as AssignmentsMap }).size).toBe(0) + }) +}) + +describe('plannedPlaceIdsForDay', () => { + it('FE-UTIL-PLANNED-008: a stay covers the nights in between, not just its two ends', () => { + const sources = { + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + } + expect(plannedPlaceIdsForDay(10, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(11, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(12, DAYS, sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(13, DAYS, sources).has(7)).toBe(false) + }) + + // The day rows are the only thing that can order a stay. Without them the + // planner falls back to the id range rather than dropping the middle nights. + it('FE-UTIL-PLANNED-009: an unloaded day list still keeps the middle of a stay', () => { + const sources = { + assignments: assignments({}), + accommodations: [{ place_id: 7, start_day_id: 10, end_day_id: 12 }], + } + expect(plannedPlaceIdsForDay(11, [], sources).has(7)).toBe(true) + expect(plannedPlaceIdsForDay(13, [], sources).has(7)).toBe(false) + }) + + it('FE-UTIL-PLANNED-010: a booking counts only on the day it names', () => { + const sources = { assignments: assignments({}), reservations: [{ place_id: 5, day_id: 11 }] } + expect(plannedPlaceIdsForDay(11, DAYS, sources).has(5)).toBe(true) + expect(plannedPlaceIdsForDay(10, DAYS, sources).has(5)).toBe(false) + }) +}) diff --git a/client/src/utils/plannedPlaces.ts b/client/src/utils/plannedPlaces.ts new file mode 100644 index 000000000..30dfe9344 --- /dev/null +++ b/client/src/utils/plannedPlaces.ts @@ -0,0 +1,88 @@ +import { isDayInAccommodationRange } from './dayOrder' +import type { AssignmentsMap, Day } from '../types' + +/** + * Which of a trip's places are "planned", i.e. have a place in the itinerary. + * + * Dragging a place onto a day is the obvious way, and for a long time it was the + * only one this answered — so a hotel linked to a booking sat under "Unplanned" + * while its name was printed in the day header two panels away (#2072). A stay is + * on days by construction (day_accommodations.start_day_id / end_day_id are both + * NOT NULL), and a booking that names a day is on the plan as much as any dragged + * pin, so both count. + * + * A booking with no day does not: it exists, but nothing says when, which is + * exactly what "unplanned" means. + * + * The three surfaces that ask this — the desktop sidebar, the map's marker set and + * the phone browser — have to agree, or the list and the pins disagree about the + * same place (#1541). Hence one function rather than three predicates. + */ + +/** A stay, as the planner and the reservation rows carry it. */ +export interface PlannedAccommodation { + place_id?: number | null + start_day_id?: number | null + end_day_id?: number | null +} + +/** A booking, reduced to what decides whether it plans its place. */ +export interface PlannedReservation { + place_id?: number | null + day_id?: number | null +} + +export interface PlannedSources { + assignments: AssignmentsMap + accommodations?: PlannedAccommodation[] + reservations?: PlannedReservation[] +} + +function addAssignmentPlaces(ids: Set, dayAssignments: { place?: { id?: number | null } | null }[]): void { + for (const a of dayAssignments) { + if (a.place?.id != null) ids.add(a.place.id) + } +} + +/** Every place the trip has a plan for, across all days. */ +export function plannedPlaceIds({ assignments, accommodations = [], reservations = [] }: PlannedSources): Set { + const ids = new Set() + for (const dayAssignments of Object.values(assignments || {})) addAssignmentPlaces(ids, dayAssignments) + for (const acc of accommodations) { + // A deleted place leaves place_id NULL behind (ON DELETE SET NULL). + if (acc.place_id != null) ids.add(acc.place_id) + } + for (const r of reservations) { + // day_id only, matching what the day plan itself renders: a booking carrying + // just an end day appears on no day at all. + if (r.place_id != null && r.day_id != null) ids.add(r.place_id) + } + return ids +} + +/** The same question for one day — what the map answers while a day is selected. */ +export function plannedPlaceIdsForDay( + dayId: number, + days: Day[], + { assignments, accommodations = [], reservations = [] }: PlannedSources, +): Set { + const ids = new Set() + addAssignmentPlaces(ids, assignments?.[String(dayId)] || []) + const day = (days || []).find(d => d.id === dayId) + for (const acc of accommodations) { + if (acc.place_id == null || acc.start_day_id == null) continue + const end = acc.end_day_id ?? acc.start_day_id + // A stay covers the nights in between, so its place is planned on each of + // them, not only on the two days it is pinned to. The day rows are the only + // thing that can order them; without them, fall back to the id range the + // rest of the planner uses. + const inRange = day + ? isDayInAccommodationRange(day, acc.start_day_id, end, days) + : dayId >= Math.min(acc.start_day_id, end) && dayId <= Math.max(acc.start_day_id, end) + if (inRange) ids.add(acc.place_id) + } + for (const r of reservations) { + if (r.place_id != null && r.day_id === dayId) ids.add(r.place_id) + } + return ids +} diff --git a/client/tests/integration/hooks/useRouteCalculation.test.ts b/client/tests/integration/hooks/useRouteCalculation.test.ts index cd2c6f8b3..d8d7c7b0c 100644 --- a/client/tests/integration/hooks/useRouteCalculation.test.ts +++ b/client/tests/integration/hooks/useRouteCalculation.test.ts @@ -339,6 +339,94 @@ describe('useRouteCalculation', () => { expect(legs).toContainEqual([`${actB.lat},${actB.lng}`, `${hotel.lat},${hotel.lng}`]); }); + // #2071 — Frankfurt → Vancouver via Reykjavik, an airport place either side. The + // booking's position lives per LEG in metadata.legs, which this hook never read, so + // the flight vanished from the waypoint list and the two airports were joined into + // one driving run across the Atlantic. + it('FE-HOOK-ROUTE-024: a layover flight breaks the run instead of joining the airports', async () => { + const fra = { lat: 50.0379, lng: 8.5622 }; + const kef = { lat: 63.985, lng: -22.6056 }; + const yvr = { lat: 49.1947, lng: -123.1792 }; + const fraPlace = buildPlace({ lat: fra.lat, lng: fra.lng }); + const yvrPlace = buildPlace({ lat: yvr.lat, lng: yvr.lng }); + + const flight = { + id: 300, type: 'flight', day_id: 1, end_day_id: 1, + endpoints: [ + { role: 'from', sequence: 0, lat: fra.lat, lng: fra.lng }, + { role: 'stop', sequence: 1, lat: kef.lat, lng: kef.lng }, + { role: 'to', sequence: 2, lat: yvr.lat, lng: yvr.lng }, + ], + metadata: JSON.stringify({ + legs: [ + { from: 'FRA', to: 'KEF', dep_time: '10:00', arr_time: '12:00', day_positions: { 1: 0.5 } }, + { from: 'KEF', to: 'YVR', dep_time: '14:00', arr_time: '16:00', day_positions: { 1: 0.6 } }, + ], + }), + }; + const before = buildAssignment({ day_id: 1, order_index: 0, place: fraPlace }); + const after = buildAssignment({ day_id: 1, order_index: 1, place: yvrPlace }); + const store = { assignments: { '1': [before, after] } } as unknown as TripStoreState; + useTripStore.setState({ + assignments: store.assignments, + reservations: [flight], + days: [{ id: 1, day_number: 1 }], + } as any); + + const { result } = renderHook(() => useRouteCalculation(store, 1, true, 'driving')); + await act(async () => {}); + + const legs = (result.current.route ?? []).map(run => run.map(p => `${p[0]},${p[1]}`)); + // The transatlantic pair is the bug: it must not appear in any run. + for (const run of legs) { + const i = run.indexOf(`${fra.lat},${fra.lng}`); + if (i >= 0) expect(run[i + 1]).not.toBe(`${yvr.lat},${yvr.lng}`); + } + expect(legs).not.toContainEqual([`${fra.lat},${fra.lng}`, `${yvr.lat},${yvr.lng}`]); + }); + + // Each leg spans its OWN pair of airports. Reading the booking's outermost two for + // every leg put a Reykjavik stop on a Vancouver → Frankfurt drive. + it('FE-HOOK-ROUTE-025: a stop between two legs routes against that leg\'s airports', async () => { + const fra = { lat: 50.0379, lng: 8.5622 }; + const kef = { lat: 63.985, lng: -22.6056 }; + const yvr = { lat: 49.1947, lng: -123.1792 }; + const cafe = buildPlace({ lat: 64.1466, lng: -21.9426 }); + + const flight = { + id: 301, type: 'flight', day_id: 1, end_day_id: 1, + endpoints: [ + { role: 'from', sequence: 0, lat: fra.lat, lng: fra.lng }, + { role: 'stop', sequence: 1, lat: kef.lat, lng: kef.lng }, + { role: 'to', sequence: 2, lat: yvr.lat, lng: yvr.lng }, + ], + metadata: JSON.stringify({ + legs: [ + { from: 'FRA', to: 'KEF', dep_time: '08:00', arr_time: '10:00', day_positions: { 1: 0.5 } }, + { from: 'KEF', to: 'YVR', dep_time: '16:00', arr_time: '18:00', day_positions: { 1: 1.5 } }, + ], + }), + }; + const stop = buildAssignment({ day_id: 1, order_index: 1, place: cafe }); + const store = { assignments: { '1': [stop] } } as unknown as TripStoreState; + useTripStore.setState({ + assignments: store.assignments, + reservations: [flight], + days: [{ id: 1, day_number: 1 }], + } as any); + + const { result } = renderHook(() => useRouteCalculation(store, 1, true, 'driving')); + await act(async () => {}); + + const legs = (result.current.route ?? []).map(run => run.map(p => `${p[0]},${p[1]}`)); + // The café is reached from Reykjavik and left towards Reykjavik. + expect(legs).toContainEqual([`${kef.lat},${kef.lng}`, `${cafe.lat},${cafe.lng}`, `${kef.lat},${kef.lng}`]); + // Neither of the far airports may appear next to it. + const flat = legs.flat(); + expect(flat).not.toContain(`${fra.lat},${fra.lng}`); + expect(flat).not.toContain(`${yvr.lat},${yvr.lng}`); + }); + it('FE-HOOK-ROUTE-015: day-1 with a first activity timed after check-in keeps the hotel → first-activity leg', async () => { // The check-in day is still a home-base loop when the first activity provably happens // at/after check-in (you dropped your bags first) — the hotel → first-stop leg remains.