diff --git a/client/src/mobile/screens/trip/map/MMapArea.tsx b/client/src/mobile/screens/trip/map/MMapArea.tsx index b55aa8b41..c2845f40d 100644 --- a/client/src/mobile/screens/trip/map/MMapArea.tsx +++ b/client/src/mobile/screens/trip/map/MMapArea.tsx @@ -92,26 +92,49 @@ function alternativesFitPadding(insets: SafeInsets): ViewportPadding { } } +type FocusPoints = readonly [number, number][] + /** A camera focus the planner is holding, and the day that was on screen when it arrived. */ interface HeldFocus { - points: readonly [number, number][] + points: FocusPoints dayId: number | null live: boolean + /** + * Every array held before this one. Weak, so the offers of a long session are not kept + * alive by it. Only ever added to, and adding twice changes nothing, so a render React + * runs twice or throws away leaves it saying the same thing. + */ + shown: WeakSet } /** - * Lets go of a pending focus once the day moves off the one it arrived with. + * Lets go of a pending focus once the day moves off the one it arrived with, and never + * takes one back that the planner hands over a second time. * * The planner keeps a point it was asked to show until its next routing round, whatever * day is picked in the meantime, and a pending focus wins over the stage's own frame. So * after "show on map" every stage swiped to afterwards stayed on that one point instead of * framing its drive. Coming back to the day does not bring the point back either: the - * traveller has moved on since, and the camera belongs to the stage they came back to. A - * new focus is a new array, held again from the day it arrives together with. + * traveller has moved on since, and the camera belongs to the stage they came back to. + * + * The same kept point is also what the planner falls back to once the fuel offers or the + * other roads for a leg close, as the very array it handed over before. Read as a new + * focus, closing either one flew the camera back to that old stop, on another day's stage + * too. So only an array never held before is a new focus. The one exception is the open + * picker's roads (`openRoads`): fuel offers asked for over an open picker take the camera + * from it, and once they close the picker is still asking about its leg. */ -function holdFocus(prev: HeldFocus, points: readonly [number, number][], dayId: number | null): HeldFocus { - if (prev.points !== points) return { points, dayId, live: true } - if (prev.live && prev.dayId !== dayId) return { points, dayId, live: false } +function holdFocus( + prev: HeldFocus, + points: FocusPoints, + dayId: number | null, + openRoads: FocusPoints, +): HeldFocus { + if (prev.points !== points) { + prev.shown.add(prev.points) + return { points, dayId, live: points === openRoads || !prev.shown.has(points), shown: prev.shown } + } + if (prev.live && prev.dayId !== dayId) return { ...prev, dayId, live: false } return prev } @@ -172,8 +195,8 @@ export default function MMapArea({ planner, shell }: MMapAreaProps) { ) // Only what the stage carries, so a trip's other 200 pins stay off a screen that - // is answering one question. Without a stage (the all-days view) every planned - // place comes back, which is what the drive looks like end to end. + // is answering one question. Without a stage (the all-days view) every place a stored + // visit stops at comes back, which is what the drive looks like end to end. // // Out of the trip store rather than `planner.mapPlaces`, which is the plan tab's map. // That list drops the pins of every day its declutter has put away, and this tab moves @@ -184,19 +207,27 @@ export default function MMapArea({ planner, shell }: MMapAreaProps) { // that the chain still draws. The category filter stays off the stage too, on purpose: // the stage is one day's chain on a map, and a stop the chain lists with no pin under its // line, hidden by a control on another tab, reads as a broken map rather than a filter. - const stagePlaces = useMemo( - () => stageMap && stage - ? tripPlaces.filter(p => p.lat != null && p.lng != null && stageMap.placeIds.has(p.id)) - : planner.roadtripMapPlaces, - [stageMap, stage, tripPlaces, planner.roadtripMapPlaces], + // + // The all-days view reads the stored visits for the same reasons, not the planner's + // planned list: that one is built from the day lists, which on a phone always leave out + // a booked night and, with the setting off, the service stops, while the drive the routing + // round draws still ends at that hotel and stops at that pump. Its pin is what opens the + // stop, so a missing one left the stop out of reach from this view. + const drivePlaceIds = useMemo( + () => new Set(Object.values(planner.storedAssignments).flat().map(visit => visit.place_id)), + [planner.storedAssignments], ) + const stagePlaces = useMemo(() => { + const pinned = stageMap && stage ? stageMap.placeIds : drivePlaceIds + return tripPlaces.filter(p => p.lat != null && p.lng != null && pinned.has(p.id)) + }, [stageMap, stage, tripPlaces, drivePlaceIds]) // Computed during render rather than in an effect, so the frame handed over below is // already the right one in the render a day change happens in. See holdFocus. const [heldFocus, setHeldFocus] = useState( - () => ({ points: planner.mapFocusPoints, dayId: planner.selectedDayId, live: true }), + () => ({ points: planner.mapFocusPoints, dayId: planner.selectedDayId, live: true, shown: new WeakSet() }), ) - const focus = holdFocus(heldFocus, planner.mapFocusPoints, planner.selectedDayId) + const focus = holdFocus(heldFocus, planner.mapFocusPoints, planner.selectedDayId, planner.alternativeFocusPoints) if (focus !== heldFocus) setHeldFocus(focus) const focusPending = focus.live && planner.mapFocusPoints.length > 0 diff --git a/client/tests/unit/mobile/trip/MMapArea.test.tsx b/client/tests/unit/mobile/trip/MMapArea.test.tsx index 2d86ba4d2..8814ed170 100644 --- a/client/tests/unit/mobile/trip/MMapArea.test.tsx +++ b/client/tests/unit/mobile/trip/MMapArea.test.tsx @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { act, render, screen } from '../../../helpers/render' import { buildPlanner, buildShell } from '../../../helpers/mobileTrip' -import { buildPlace } from '../../../helpers/factories' +import { buildAssignment, buildPlace } from '../../../helpers/factories' import type { AccessSpur, RoadtripDay } from '@trek/shared/roadtrip' import type { MTripShellApi, TripPlanner } from '../../../../src/mobile/screens/trip/MTripShell' import type { CompassMap } from '../../../../src/components/Map/MapCompassPill' @@ -14,7 +14,7 @@ import type { AlternativeOverlay } from '../../../../src/components/Roadtrip/alt import type { LegAlternatives } from '../../../../src/components/Roadtrip/useRouteAlternatives' import { RT_ALT_BAR_LIFT } from '../../../../src/mobile/screens/trip/roadtrip/useMRtAlternatives' -// FE-MOB-MAPAREA-001 to FE-MOB-MAPAREA-032 +// FE-MOB-MAPAREA-001 to FE-MOB-MAPAREA-035 // // The stage's pins come out of the trip store rather than the planner's map list, so the // stage fixtures seed the store and leave `mapPlaces` to stand for what the plan tab shows. @@ -97,8 +97,13 @@ function stagePlanner(selectedDayId: number | null): TripPlanner { return buildPlanner({ selectedDayId, mapPlaces: [buildPlace({ id: 11 }), buildPlace({ id: 12 }), buildPlace({ id: 99 })], - // Every planned place, which is what the all days view draws. - roadtripMapPlaces: [buildPlace({ id: 11 }), buildPlace({ id: 12 })], + // The stored visits, which are what the all days view draws: 99 is on no day. + storedAssignments: { + '3': [ + buildAssignment({ day_id: 3, place: buildPlace({ id: 11 }) }), + buildAssignment({ day_id: 3, place: buildPlace({ id: 12 }) }), + ], + }, roadtripRoutes: { ...base.roadtripRoutes, days: [stageDay()], @@ -386,7 +391,8 @@ describe('MMapArea', () => { const allDays = { ...planner, selectedDayId: null } rerender() - expect(mocks.props.places).toBe(allDays.roadtripMapPlaces) + expect((mocks.props.places as Place[]).map(p => p.id)).toEqual([11, 12]) + expect(mocks.props.places).toContain(renamed) expect(mocks.props.focusPoints).toEqual([]) // Off the tab the stage hands nothing through, so the plan tab frames itself again. @@ -641,4 +647,97 @@ describe('MMapArea', () => { unmount() } }) + + it('FE-MOB-MAPAREA-033: a point the planner hands back once offers or roads close does not take the camera again', () => { + const planner = drivePlanner(2) + const shell = stageShell() + const view = (dayId: number, mapFocusPoints: [number, number][], alternativeFocusPoints: [number, number][] = []) => + + const point: [number, number][] = [[50, 8]] + const { rerender } = render(view(2, point)) + expect(mocks.props.focusPoints).toBe(point) + + // Swiped on: the planner still keeps the point from day 2, and this stage frames itself. + rerender(view(3, point)) + const stageFrame = mocks.props.focusPoints + expect(stageFrame).not.toBe(point) + + // The fuel offers take the camera, and closing them hands the same point back. The chips + // still say day 3, so flying to day 2's stop would leave the map on a stage nobody picked. + const offers: [number, number][] = [[53.4, 9.7]] + rerender(view(3, offers)) + expect(mocks.props.focusPoints).toBe(offers) + rerender(view(3, point)) + expect(mocks.props.focusPoints).toBe(stageFrame) + + // On its own day as well: the roads for a leg, cancelled, give the frame back to the stage. + const shown: [number, number][] = [[53.87, 10.69]] + rerender(view(3, shown)) + expect(mocks.props.focusPoints).toBe(shown) + const roads: [number, number][] = [[53.55, 9.99], [53.7, 10.2], [53.87, 10.69]] + rerender(view(3, roads, roads)) + expect(mocks.props.focusPoints).toBe(roads) + rerender(view(3, shown)) + expect(mocks.props.focusPoints).toBe(stageFrame) + + // And every time after: a second round of offers is new, the point behind it is not. + const again: [number, number][] = [[53.3, 9.6]] + rerender(view(3, again)) + expect(mocks.props.focusPoints).toBe(again) + rerender(view(3, shown)) + expect(mocks.props.focusPoints).toBe(stageFrame) + }) + + it('FE-MOB-MAPAREA-034: fuel offers asked over an open picker give the camera back to its roads when they close', () => { + const planner = drivePlanner(3) + const shell = stageShell() + const roads: [number, number][] = [[53.55, 9.99], [53.7, 10.2], [53.87, 10.69]] + const view = (mapFocusPoints: [number, number][], alternativeFocusPoints: [number, number][]) => + + const { rerender } = render(view(roads, roads)) + expect(mocks.props.focusPoints).toBe(roads) + + const offers: [number, number][] = [[53.4, 9.7]] + rerender(view(offers, roads)) + expect(mocks.props.focusPoints).toBe(offers) + + // The picker is still open and still asking about its leg, so its roads are framed again. + rerender(view(roads, roads)) + expect(mocks.props.focusPoints).toBe(roads) + + // Closed, nothing is pending and the stage frames itself. + const closed: [number, number][] = [] + rerender(view(closed, closed)) + expect(mocks.props.focusPoints).toEqual([[53.55, 9.99], [53.87, 10.69], [53.55, 9.99]]) + }) + + it('FE-MOB-MAPAREA-035: the all days view pins every stored visit, a booked night and a hidden service stop included', () => { + const hotel = buildPlace({ id: 20 }) + const pump = buildPlace({ id: 21, stop_type: 'fuel' } as Partial) + const base = drivePlanner(null) + // The places browser on 'unplanned' with a category picked, which empties the plan tab's map. + seedStore(useTripStore, { + places: [buildPlace({ id: 11 }), hotel, pump, buildPlace({ id: 99 })], + placesFilter: 'unplanned', + placesCategoryFilter: new Set(['9']), + }) + const planner = { + ...base, + mapPlaces: [], + // The planner's planned list on a phone: its day lists leave the booked night and the + // hidden pump out, so it is not what the drive the routing round draws stops at. + roadtripMapPlaces: [buildPlace({ id: 11 })], + storedAssignments: { + '3': [ + buildAssignment({ day_id: 3, place: buildPlace({ id: 11 }) }), + buildAssignment({ day_id: 3, place: pump }), + ], + '4': [buildAssignment({ day_id: 4, place: hotel, accommodation_id: 5 })], + }, + } as TripPlanner + render() + + expect((mocks.props.places as Place[]).map(p => p.id)).toEqual([11, 20, 21]) + expect(mocks.props.places).not.toBe(planner.roadtripMapPlaces) + }) })