From 1db227cb5b5495d6091918e5a95c0d1deb6dc16d Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 01:22:06 +0000 Subject: [PATCH] refactor(client): the map marker set and the day order badges move into useMapPlaces The list filters the map shares with the sidebars, the declutter of collapsed days, the compact and pending marker flags, the Tracks filter fallback, and the selected day's order badges and fit list move out of useTripPlanner into useMapPlaces. useTripPlanner calls the hook where the marker set used to sit, right after the trip's websocket, so the store subscriptions and the Tracks fallback still run in the same order and the planner returns the same object. The order badges and the fit list are plain memos, so computing them earlier changes nothing. --- .../pages/tripPlanner/useMapPlaces.test.ts | 183 ++++++++++++++++++ client/src/pages/tripPlanner/useMapPlaces.ts | 128 ++++++++++++ .../src/pages/tripPlanner/useTripPlanner.ts | 109 +---------- 3 files changed, 318 insertions(+), 102 deletions(-) create mode 100644 client/src/pages/tripPlanner/useMapPlaces.test.ts create mode 100644 client/src/pages/tripPlanner/useMapPlaces.ts diff --git a/client/src/pages/tripPlanner/useMapPlaces.test.ts b/client/src/pages/tripPlanner/useMapPlaces.test.ts new file mode 100644 index 000000000..0a9fb9a90 --- /dev/null +++ b/client/src/pages/tripPlanner/useMapPlaces.test.ts @@ -0,0 +1,183 @@ +// FE-TP-MAPPLACES-001 to FE-TP-MAPPLACES-010 +// +// Which places the planner's map draws, driven straight through useMapPlaces: the +// list filters it shares with the sidebars through the trip store, the declutter of +// collapsed days, the marker flags, the Tracks fallback and the selected day's order +// badges and fit list. +import { act, renderHook } from '@testing-library/react' +import { useTripStore } from '../../store/tripStore' +import { useSettingsStore } from '../../store/settingsStore' +import { resetAllStores, seedStore } from '../../../tests/helpers/store' +import { buildAssignment, buildDay, buildPlace, buildReservation } from '../../../tests/helpers/factories' +import type { Accommodation, AssignmentsMap, Day, Place, Reservation } from '../../types' +import { useMapPlaces } from './useMapPlaces' + +interface Props { + places: Place[] + assignments: AssignmentsMap + selectedDayId: number | null + days: Day[] + tripAccommodations: Accommodation[] + reservations: Reservation[] + toursEnabled: boolean +} + +// Four places: one on day 1, one on day 2 and two in the pool, one of which has no +// position for the map to draw. The day 1 place is planned on day 2 as well, so +// collapsing either day alone must not hide it. +const museum = buildPlace({ id: 1, name: 'Museum' }) +const harbour = buildPlace({ id: 2, name: 'Harbour' }) +const bakery = buildPlace({ id: 3, name: 'Bakery' }) +const nowhere = buildPlace({ id: 4, name: 'Nowhere', lat: null, lng: null }) +const days = [buildDay({ id: 1, day_number: 1 }), buildDay({ id: 2, day_number: 2 })] +const assignments: AssignmentsMap = { + 1: [buildAssignment({ id: 11, day_id: 1, place: museum, order_index: 0 })], + 2: [ + buildAssignment({ id: 21, day_id: 2, place: harbour, order_index: 0 }), + buildAssignment({ id: 22, day_id: 2, place: museum, order_index: 1 }), + ], +} + +function renderMapPlaces(over: Partial = {}) { + const initialProps: Props = { + places: [museum, harbour, bakery, nowhere], + assignments, + selectedDayId: null, + days, + tripAccommodations: [], + reservations: [], + toursEnabled: false, + ...over, + } + return renderHook((props: Props) => useMapPlaces(props), { initialProps }) +} + +const ids = (places: { id: number }[]) => places.map(p => p.id) + +beforeEach(() => { + resetAllStores() +}) + +describe('useMapPlaces', () => { + it('FE-TP-MAPPLACES-001: with no filter the map draws every place that has a position, unchanged', () => { + const { result } = renderMapPlaces() + + expect(ids(result.current.mapPlaces)).toEqual([1, 2, 3]) + expect(result.current.mapPlaces[0]).toBe(museum) + expect(result.current.expandedDayIds).toBeNull() + }) + + it('FE-TP-MAPPLACES-002: a collapsed day takes its stops off the map unless an open day still holds them', () => { + // A visit whose place is gone (still syncing, or deleted elsewhere) hides nothing. + const placeless = (id: number, dayId: number) => buildAssignment({ id, day_id: dayId, place: null } as never) + const { result } = renderMapPlaces({ + assignments: { 1: [...assignments[1], placeless(12, 1)], 2: [...assignments[2], placeless(23, 2)] }, + }) + + act(() => { result.current.setExpandedDayIds(new Set([2])) }) + expect(ids(result.current.mapPlaces)).toEqual([1, 2, 3]) + + act(() => { result.current.setExpandedDayIds(new Set([1])) }) + expect(ids(result.current.mapPlaces)).toEqual([1, 3]) + + act(() => { result.current.setExpandedDayIds(new Set()) }) + expect(ids(result.current.mapPlaces)).toEqual([3]) + }) + + it('FE-TP-MAPPLACES-003: under the planned filter a collapsed day keeps its places on the map', () => { + seedStore(useTripStore, { placesFilter: 'planned' }) + const { result } = renderMapPlaces() + + act(() => { result.current.setExpandedDayIds(new Set()) }) + + expect(ids(result.current.mapPlaces)).toEqual([1, 2]) + }) + + it('FE-TP-MAPPLACES-004: planned follows the open day, unplanned always reads the whole trip', () => { + seedStore(useTripStore, { placesFilter: 'planned' }) + const { result, rerender } = renderMapPlaces({ selectedDayId: 1 }) + expect(ids(result.current.mapPlaces)).toEqual([1]) + + act(() => { useTripStore.setState({ placesFilter: 'unplanned' }) }) + rerender({ + places: [museum, harbour, bakery, nowhere], assignments, selectedDayId: 1, days, + tripAccommodations: [], reservations: [], toursEnabled: false, + }) + expect(ids(result.current.mapPlaces)).toEqual([3]) + }) + + it('FE-TP-MAPPLACES-005: a stay or a booking on a day plans its place for the filters', () => { + seedStore(useTripStore, { placesFilter: 'unplanned' }) + const stay = { id: 50, place_id: 3, start_day_id: 1, end_day_id: 2 } as Accommodation + const { result } = renderMapPlaces({ tripAccommodations: [stay] }) + + expect(ids(result.current.mapPlaces)).toEqual([]) + }) + + it('FE-TP-MAPPLACES-006: the category and rating filters the lists set narrow the markers too', () => { + const rated = buildPlace({ id: 5, category_id: 7, rating_avg: 4.5 } as Partial) + seedStore(useTripStore, { placesCategoryFilter: new Set(['7']), placesRatingFilter: 4 }) + const { result } = renderMapPlaces({ places: [museum, rated] }) + + expect(ids(result.current.mapPlaces)).toEqual([5]) + }) + + it('FE-TP-MAPPLACES-007: compact unplanned and pending stays mark their markers, planned ones stay as they are', () => { + seedStore(useSettingsStore, { settings: { map_compact_unplanned: true } }) + const stay = { id: 60, place_id: 2, start_day_id: 2, end_day_id: 2 } as Accommodation + const booking = buildReservation({ id: 61, accommodation_id: 60, status: 'pending' } as Partial) + const { result } = renderMapPlaces({ tripAccommodations: [stay], reservations: [booking] }) + + const [first, second, third] = result.current.mapPlaces + expect(first).toBe(museum) + expect(second).toEqual({ ...harbour, _compact: false, _pending: true }) + expect(third).toEqual({ ...bakery, _compact: true, _pending: false }) + }) + + it('FE-TP-MAPPLACES-008: the Tracks filter falls back to all once nothing carries a track, or tours take tracks over', () => { + const trail = buildPlace({ id: 6, route_geometry: '[[46.9,7.4],[46.8,7.5]]' } as Partial) + seedStore(useTripStore, { placesFilter: 'tracks' }) + + const kept = renderMapPlaces({ places: [museum, trail] }) + expect(useTripStore.getState().placesFilter).toBe('tracks') + expect(ids(kept.result.current.mapPlaces)).toEqual([6]) + kept.unmount() + + renderMapPlaces({ places: [museum, trail], toursEnabled: true }) + expect(useTripStore.getState().placesFilter).toBe('all') + + seedStore(useTripStore, { placesFilter: 'tracks' }) + renderMapPlaces({ places: [museum] }) + expect(useTripStore.getState().placesFilter).toBe('all') + }) + + it('FE-TP-MAPPLACES-009: the order badges follow the day order and pass over service stops', () => { + const fuel = buildPlace({ id: 7, stop_type: 'fuel' } as Partial) + const day: AssignmentsMap = { + 3: [ + buildAssignment({ id: 31, day_id: 3, place: harbour, order_index: 2 }), + buildAssignment({ id: 32, day_id: 3, place: fuel, order_index: 1 }), + buildAssignment({ id: 33, day_id: 3, place: museum, order_index: 0 }), + buildAssignment({ id: 34, day_id: 3, place: museum, order_index: 3 }), + buildAssignment({ id: 35, day_id: 3, place: null, order_index: 4 } as never), + ], + } + + expect(renderMapPlaces({ assignments: day }).result.current.dayOrderMap).toEqual({}) + expect(renderMapPlaces({ assignments: day, selectedDayId: 9 }).result.current.dayOrderMap).toEqual({}) + expect(renderMapPlaces({ assignments: day, selectedDayId: 3 }).result.current.dayOrderMap).toEqual({ 1: [1, 3], 2: [2] }) + }) + + it('FE-TP-MAPPLACES-010: the map fits to the selected day\'s places that have a position', () => { + const day: AssignmentsMap = { + 3: [ + buildAssignment({ id: 41, day_id: 3, place: nowhere, order_index: 0 }), + buildAssignment({ id: 42, day_id: 3, place: bakery, order_index: 1 }), + ], + } + + expect(renderMapPlaces({ assignments: day }).result.current.dayPlaces).toEqual([]) + expect(renderMapPlaces({ assignments: day, selectedDayId: 9 }).result.current.dayPlaces).toEqual([]) + expect(renderMapPlaces({ assignments: day, selectedDayId: 3 }).result.current.dayPlaces).toEqual([bakery]) + }) +}) diff --git a/client/src/pages/tripPlanner/useMapPlaces.ts b/client/src/pages/tripPlanner/useMapPlaces.ts new file mode 100644 index 000000000..79ac2990b --- /dev/null +++ b/client/src/pages/tripPlanner/useMapPlaces.ts @@ -0,0 +1,128 @@ +import { useEffect, useMemo, useState } from 'react' +import { isServiceStopType } from '../../components/Roadtrip/roadtripModel' +import { useSettingsStore } from '../../store/settingsStore' +import { useTripStore, type TripStoreState } from '../../store/tripStore' +import type { Accommodation, Place } from '../../types' +import { pendingStayPlaceIds } from '../../utils/pendingStays' +import { matchesPlacesFilter } from '../../utils/placesFilter' +import { plannedPlaceIds, plannedPlaceIdsForDay } from '../../utils/plannedPlaces' + +interface MapPlacesOptions { + places: Place[] + assignments: TripStoreState['assignments'] + selectedDayId: TripStoreState['selectedDayId'] + days: TripStoreState['days'] + tripAccommodations: Accommodation[] + reservations: TripStoreState['reservations'] + toursEnabled: boolean +} + +/** + * Which places the planner's map draws and how: the list filters shared through the + * store, the declutter of collapsed days, the compact and pending markers, the Tracks + * filter falling back once no place carries a track, and the selected day's order + * badges and the places the map fits to. + */ +export function useMapPlaces(options: MapPlacesOptions) { + const { places, assignments, selectedDayId, days, tripAccommodations, reservations, toursEnabled } = options + // Same filter the places sidebar renders, shared via the store so tab + // switches can't desync the marker set from the filter UI (#1541). + const placesFilter = useTripStore((s) => s.placesFilter) + const placesCategoryFilter = useTripStore((s) => s.placesCategoryFilter) + const placesRatingFilter = useTripStore((s) => s.placesRatingFilter) + + const [expandedDayIds, setExpandedDayIds] = useState | null>(null) + + const compactUnplanned = useSettingsStore(s => s.settings.map_compact_unplanned === true) + const mapPlaces = useMemo(() => { + // Build set of place IDs assigned to collapsed days + const hiddenPlaceIds = new Set() + if (expandedDayIds) { + for (const [dayId, dayAssignments] of Object.entries(assignments)) { + if (!expandedDayIds.has(Number(dayId))) { + for (const a of dayAssignments) { + if (a.place?.id) hiddenPlaceIds.add(a.place.id) + } + } + } + // Don't hide places that are also assigned to an expanded day + for (const [dayId, dayAssignments] of Object.entries(assignments)) { + if (expandedDayIds.has(Number(dayId))) { + for (const a of dayAssignments) { + if (a.place?.id) hiddenPlaceIds.delete(a.place.id) + } + } + } + } + + // Planned place IDs: needed by both the 'unplanned' filter (exclude them) and + // the new 'planned' filter (keep only them). With a day selected, 'planned' + // follows it like the other filters do; with no day selected it keeps showing + // 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' + ? (placesFilter === 'planned' && selectedDayId + ? plannedPlaceIdsForDay(selectedDayId, days, { assignments, accommodations: tripAccommodations, reservations }) + : plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations })) + : null + + const compactIds = compactUnplanned ? plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations }) : null + const pendingIds = pendingStayPlaceIds(tripAccommodations, reservations) + const filterState = { filter: placesFilter, categoryFilters: placesCategoryFilter, ratingFilter: placesRatingFilter } + return places.filter(p => { + if (!p.lat || !p.lng) return false + // Collapsed-day declutter hides a day's stops on every filter EXCEPT 'planned': + // there the user asked to see the whole plan on the map, so a collapsed day + // must not drop its planned places. + if (placesFilter !== 'planned' && hiddenPlaceIds.has(p.id)) return false + // Pool, categories and rating floor: the very matcher the lists use (#1541). + return matchesPlacesFilter(p, filterState, { plannedIds }) + }).map(p => { + // How the map tells a place apart (#2024, #2281); untouched places keep their identity. + const compact = !!compactIds && !compactIds.has(p.id) + const pending = pendingIds.has(p.id) + return compact || pending ? { ...p, _compact: compact, _pending: pending } : p + }) + }, [places, placesCategoryFilter, placesFilter, placesRatingFilter, assignments, expandedDayIds, selectedDayId, days, tripAccommodations, reservations, compactUnplanned]) + + // The "Tracks" pool is only offered while a place carries a track. When the last + // one goes (deleted here or by a collaborator), fall back to "all". That happens here + // because the planner's hooks are mounted whatever is on screen: the lists that used + // to do it are not mounted while the phone map is in front, and the map would sit + // empty under a "Tracks" filter no control offers any more. + const setPlacesFilter = useTripStore((s) => s.setPlacesFilter) + const hasTracks = useMemo(() => !toursEnabled && places.some(p => p.route_geometry), [places, toursEnabled]) + useEffect(() => { + if (placesFilter === 'tracks' && !hasTracks) setPlacesFilter('all') + }, [placesFilter, hasTracks, setPlacesFilter]) + + // Build placeId → order-number map from the selected day's assignments. A service + // stop is passed over rather than counted: the map draws it without a badge and the + // rail gives it no number, so a number spent on it left the pin after a fuel stop + // wearing "3" where the rail said "2". + const dayOrderMap = useMemo(() => { + if (!selectedDayId) return {} + const da = assignments[String(selectedDayId)] || [] + const sorted = [...da].sort((a, b) => a.order_index - b.order_index) + const map = {} + let counted = 0 + sorted.forEach(a => { + if (!a.place?.id || isServiceStopType(a.place.stop_type)) return + counted += 1 + if (!map[a.place.id]) map[a.place.id] = [] + map[a.place.id].push(counted) + }) + return map + }, [selectedDayId, assignments]) + + // Places assigned to selected day (with coords), used for map fitting + const dayPlaces = useMemo(() => { + if (!selectedDayId) return [] + const da = assignments[String(selectedDayId)] || [] + return da.map(a => a.place).filter(p => p?.lat && p?.lng) + }, [selectedDayId, assignments]) + + return { expandedDayIds, setExpandedDayIds, mapPlaces, dayOrderMap, dayPlaces } +} + +export type MapPlaces = ReturnType diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 5827412ee..e70aaa5be 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -79,13 +79,11 @@ import { type StoredConnections, } from '../../utils/connectionsVisibility' import { usePlaceLanguage } from '../../hooks/usePlaceLanguage' -import { plannedPlaceIds, plannedPlaceIdsForDay } from '../../utils/plannedPlaces' -import { matchesPlacesFilter } from '../../utils/placesFilter' -import { pendingStayPlaceIds } from '../../utils/pendingStays' import { useDayDelete } from './useDayDelete' import { useDayAdd } from './useDayAdd' import { usePlannerDialogs } from './usePlannerDialogs' import { usePlannerMapView } from './usePlannerMapView' +import { useMapPlaces } from './useMapPlaces' import { useIsPhone } from '../../mobile/useIsPhone' /** Stable empty list so the road trip hook stays inert while its mode is off. */ @@ -95,9 +93,9 @@ const EMPTY_RESERVATIONS: Reservation[] = [] /** * Trip planner page logic, the big one. Owns the trip store wiring, addon * gating, accommodations/members loading, the tab + resizable-panel + selection - * state, every place/assignment/reservation/transport CRUD handler (with undo), - * the map filters/derivations and the splash gate. TripPlannerPage stays a - * wiring container that lays out the day/map/places panes and modals. + * state, every place/assignment/reservation/transport CRUD handler (with undo) + * and the splash gate. TripPlannerPage stays a wiring container that lays out + * the day/map/places panes and modals. * Behaviour is identical to the previous in-component logic. * * Self-contained parts live in the use* sub-hooks beside this file. Each one is @@ -510,76 +508,9 @@ export function useTripPlanner() { useTripWebSocket(tripId, invalidateTourPlaceIds) - // Same filter the places sidebar renders — shared via the store so tab - // switches can't desync the marker set from the filter UI (#1541). - const placesFilter = useTripStore((s) => s.placesFilter) - const placesCategoryFilter = useTripStore((s) => s.placesCategoryFilter) - const placesRatingFilter = useTripStore((s) => s.placesRatingFilter) - - const [expandedDayIds, setExpandedDayIds] = useState | null>(null) - - const compactUnplanned = useSettingsStore(s => s.settings.map_compact_unplanned === true) - const mapPlaces = useMemo(() => { - // Build set of place IDs assigned to collapsed days - const hiddenPlaceIds = new Set() - if (expandedDayIds) { - for (const [dayId, dayAssignments] of Object.entries(assignments)) { - if (!expandedDayIds.has(Number(dayId))) { - for (const a of dayAssignments) { - if (a.place?.id) hiddenPlaceIds.add(a.place.id) - } - } - } - // Don't hide places that are also assigned to an expanded day - for (const [dayId, dayAssignments] of Object.entries(assignments)) { - if (expandedDayIds.has(Number(dayId))) { - for (const a of dayAssignments) { - if (a.place?.id) hiddenPlaceIds.delete(a.place.id) - } - } - } - } - - // Planned place IDs — needed by both the 'unplanned' filter (exclude them) and - // the new 'planned' filter (keep only them). With a day selected, 'planned' - // follows it like the other filters do; with no day selected it keeps showing - // 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' - ? (placesFilter === 'planned' && selectedDayId - ? plannedPlaceIdsForDay(selectedDayId, days, { assignments, accommodations: tripAccommodations, reservations }) - : plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations })) - : null - - const compactIds = compactUnplanned ? plannedPlaceIds({ assignments, accommodations: tripAccommodations, reservations }) : null - const pendingIds = pendingStayPlaceIds(tripAccommodations, reservations) - const filterState = { filter: placesFilter, categoryFilters: placesCategoryFilter, ratingFilter: placesRatingFilter } - return places.filter(p => { - if (!p.lat || !p.lng) return false - // Collapsed-day declutter hides a day's stops on every filter EXCEPT 'planned': - // there the user asked to see the whole plan on the map, so a collapsed day - // must not drop its planned places. - if (placesFilter !== 'planned' && hiddenPlaceIds.has(p.id)) return false - // Pool, categories and rating floor: the very matcher the lists use (#1541). - return matchesPlacesFilter(p, filterState, { plannedIds }) - }).map(p => { - // How the map tells a place apart (#2024, #2281); untouched places keep their identity. - const compact = !!compactIds && !compactIds.has(p.id) - const pending = pendingIds.has(p.id) - return compact || pending ? { ...p, _compact: compact, _pending: pending } : p - }) - }, [places, placesCategoryFilter, placesFilter, placesRatingFilter, assignments, expandedDayIds, selectedDayId, days, tripAccommodations, reservations, compactUnplanned]) - - // The "Tracks" pool is only offered while a place carries a track. When the last - // one goes (deleted here or by a collaborator), fall back to "all" — here, because - // this hook is mounted whatever is on screen: the lists that used to do it are not - // mounted while the phone map is in front, and the map would sit empty under a - // "Tracks" filter no control offers any more. - const setPlacesFilter = useTripStore((s) => s.setPlacesFilter) - const hasTracks = useMemo(() => !toursEnabled && places.some(p => p.route_geometry), [places, toursEnabled]) - useEffect(() => { - if (placesFilter === 'tracks' && !hasTracks) setPlacesFilter('all') - }, [placesFilter, hasTracks, setPlacesFilter]) + const { expandedDayIds, setExpandedDayIds, mapPlaces, dayOrderMap, dayPlaces } = useMapPlaces({ + places, assignments, selectedDayId, days, tripAccommodations, reservations, toursEnabled, + }) const { route, routeWalking, routeSegments, routeVias, routeInfo, setRoute, setRouteInfo, updateRouteForDay } = useRouteCalculation({ assignments } as any, selectedDayId, routeShown, routeProfile, tripAccommodations) // Road trip mode already draws the whole trip its own way, so the overview stands @@ -2765,32 +2696,6 @@ export function useTripPlanner() { ? inspectorStay(roadtripRoutes.days, endDayStop, selectedPlace, can('place_edit', trip) ? editRoadtripStay : undefined) : undefined - // Build placeId → order-number map from the selected day's assignments. A service - // stop is passed over rather than counted: the map draws it without a badge and the - // rail gives it no number, so a number spent on it left the pin after a fuel stop - // wearing "3" where the rail said "2". - const dayOrderMap = useMemo(() => { - if (!selectedDayId) return {} - const da = assignments[String(selectedDayId)] || [] - const sorted = [...da].sort((a, b) => a.order_index - b.order_index) - const map = {} - let counted = 0 - sorted.forEach(a => { - if (!a.place?.id || isServiceStopType(a.place.stop_type)) return - counted += 1 - if (!map[a.place.id]) map[a.place.id] = [] - map[a.place.id].push(counted) - }) - return map - }, [selectedDayId, assignments]) - - // Places assigned to selected day (with coords) — used for map fitting - const dayPlaces = useMemo(() => { - if (!selectedDayId) return [] - const da = assignments[String(selectedDayId)] || [] - return da.map(a => a.place).filter(p => p?.lat && p?.lng) - }, [selectedDayId, assignments]) - const mapTileUrl = useTileUrl(OFM_POSITRON) const fontStyle = { fontFamily: "var(--font-system)" }