diff --git a/client/src/components/Tours/planner/TourPlannerPanels.test.tsx b/client/src/components/Tours/planner/TourPlannerPanels.test.tsx index ba89f124e..99b6dc330 100644 --- a/client/src/components/Tours/planner/TourPlannerPanels.test.tsx +++ b/client/src/components/Tours/planner/TourPlannerPanels.test.tsx @@ -13,11 +13,15 @@ import { publishTourFerrataCrossings } from '../../../store/tourFerrataCrossingS import { TourPlannerRail, TourPlannerToursRail } from './TourPlannerPanels' import { hikePlannerDefaults } from '../../../../tests/helpers/tourPlannerStub' import { tourRepo } from '../../../repo/tourRepo' +import { useTripStore } from '../../../store/tripStore' +import type { TourStage } from '../stages/tourStages' vi.mock('../../../../../server/src/config', () => { throw new Error('Config initialization is forbidden in client tests') }) vi.mock('../../../../../server/src/db/database', () => { throw new Error('Legacy database imports are forbidden in client tests') }) vi.mock('better-sqlite3', () => { throw new Error('SQLite imports are forbidden in client tests') }) // The checklist card is its own suite's; here it only has to sit where the rails put it. +// The stops of a tour's stages are named by a reverse lookup; here they go without names. +vi.mock('./useWaypointPlaceNames', () => ({ useWaypointPlaceNames: () => ({}) })) vi.mock('../paddle/PaddleCheckCard', () => ({ PaddleCheckCard: ({ checklist }: { checklist: PaddleChecklist }) => (
@@ -139,6 +143,62 @@ describe('TourPlannerToursRail', () => { expect(screen.getByRole('button', { name: 'Hide stages' })).toHaveAttribute('aria-expanded', 'true') }) + it('TOUR-PLANNER-RAIL-023: MDT-02 a stage asked for while the changes are kept is let go, and shown once they are discarded', async () => { + seedStore(useTripStore, { trip: { id: 9 } as never }) + const hutToHut = { ...tour, place_id: 43, name: 'Hut to hut', stage_count: 2, revision: 1 } + vi.spyOn(tourRepo, 'detail').mockResolvedValue({ + tour: hutToHut, + waypoints: [ + { lat: 47, lng: 11, role: 'start', sequence: 0, key: 'stop-start' }, + { lat: 47.01, lng: 11, role: 'via', sequence: 1, key: 'stop-hut', stage_end: true }, + { lat: 47.03, lng: 11, role: 'end', sequence: 2, key: 'stop-end' }, + ], + }) + const stage = (number: number, fromId: string, toId: string): TourStage => ({ + number, from: number - 1, to: number, fromId, toId, + distanceKm: null, gain: null, loss: null, minutes: null, fromM: null, toM: null, overDay: false, + }) + // The hut tour as the planner holds it once opened: its points in, its two stages cut from them. + const openedHut = () => planner({ + mode: { type: 'edit-saved', placeId: 43 }, + editingPlaceId: 43, + route: null, + waypoints: [ + { id: 'p1', lat: 47, lng: 11, role: 'start', key: 'stop-start' }, + { id: 'p2', lat: 47.01, lng: 11, role: 'via', key: 'stop-hut', stageEnd: true }, + { id: 'p3', lat: 47.03, lng: 11, role: 'end', key: 'stop-end' }, + ], + stages: { ...hikePlannerDefaults().stages, multiDay: true, endCount: 1, stages: [stage(1, 'p1', 'p2'), stage(2, 'p2', 'p3')] }, + setSelectedWaypointId: vi.fn(), + setRouteProfileFocus: vi.fn(), + } as Partial) + const dirty = planner({ mode: { type: 'edit-saved', placeId: 42 }, editingPlaceId: 42, hasUnsavedChanges: true }) + const rail = (controller: TourPlannerController) => + const view = render(rail(dirty)) + await userEvent.click(screen.getByRole('button', { name: 'Show stages' })) + const openSecondStage = async () => { + const items = await screen.findAllByTestId('tour-stage-item') + await userEvent.click(within(items[1]).getByTestId('tour-stage-open')) + } + + await openSecondStage() + expect(screen.getByRole('heading', { name: 'Discard unsaved tour changes?' })).toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + expect(dirty.openTour).not.toHaveBeenCalled() + // The hut tour opened later from its row shows no stage nobody asked for then. + const kept = openedHut() + view.rerender(rail(kept)) + expect(kept.setSelectedWaypointId).not.toHaveBeenCalled() + + view.rerender(rail(dirty)) + await openSecondStage() + fireEvent.click(screen.getByRole('button', { name: 'Discard changes' })) + expect(dirty.openTour).toHaveBeenCalledWith(hutToHut) + const discarded = openedHut() + view.rerender(rail(discarded)) + expect(discarded.setSelectedWaypointId).toHaveBeenCalledWith('p3') + }) + it('RS-02: keeps calculated Walking time visible in the right Tour metrics rail', () => { render( state.settings.distance_unit) const tripId = useTripStore(state => state.trip?.id) const [pendingTour, setPendingTour] = useState(null) + // Set by the dialog's confirm, which closes it as well, so its close tells a discard from a kept draft. + const discarding = useRef(false) const dayAssignment = useTourDayAssignment({ canAssign, onAssignToDay }) // STA-01: one filter for the list and the summary above it, the same as the plan's tour list. const filters = useTourListFilters(tours) @@ -780,7 +782,10 @@ export function TourPlannerToursRail({ planner, tours, days, loading, onAssignTo const openTour = async (tour: TourListItem) => { const opened = await planner.openTour(tour) - if (!opened) toast.error(t('tours.planner.openFailed')) + if (opened) return + toast.error(t('tours.planner.openFailed')) + // MDT-02: a stage asked for with this tour is not shown on a later open of it. + stageRail.cancel() } const selectTour = (tour: TourListItem) => { @@ -899,9 +904,15 @@ export function TourPlannerToursRail({ planner, tours, days, loading, onAssignTo setPendingTour(null)} + onClose={() => { + setPendingTour(null) + // MDT-02: the dialog closes after a confirm too; only kept changes let the stage asked for go. + if (!discarding.current) stageRail.cancel() + discarding.current = false + }} onConfirm={() => { const tour = pendingTour + discarding.current = true setPendingTour(null) if (tour) selectTour(tour) }} diff --git a/client/src/components/Tours/stages/stageOverview.test.tsx b/client/src/components/Tours/stages/stageOverview.test.tsx index def9cca67..856031409 100644 --- a/client/src/components/Tours/stages/stageOverview.test.tsx +++ b/client/src/components/Tours/stages/stageOverview.test.tsx @@ -1,24 +1,24 @@ -// FE-TOUR-STAGE-OVERVIEW-001 to FE-TOUR-STAGE-OVERVIEW-015: a multi-day tour unfolded in the list, its +// FE-TOUR-STAGE-OVERVIEW-001 to FE-TOUR-STAGE-OVERVIEW-017: a multi-day tour unfolded in the list, its // stages on days of the trip, and the day plan naming the stage a day holds (MDT-02). -import type { TourListItem, TourWaypoint } from '@trek/shared'; import { act, fireEvent, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import type { TourListItem, TourWaypoint } from '@trek/shared'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { buildDay, buildPlace } from '../../../../tests/helpers/factories'; import { render, screen, within } from '../../../../tests/helpers/render'; import { resetAllStores, seedStore } from '../../../../tests/helpers/store'; -import { DayPlanPlaceSubline } from '../../Planner/DayPlanPlaceSubline'; import { tourRepo } from '../../../repo/tourRepo'; import { useSettingsStore } from '../../../store/settingsStore'; import { useTripStore } from '../../../store/tripStore'; import type { AssignmentsMap } from '../../../types'; +import { DayPlanPlaceSubline } from '../../Planner/DayPlanPlaceSubline'; import type { TourPlannerController } from '../planner/useTourPlanner'; import TourListRow from '../TourListRow'; -import { savedStagePoints, savedTourStagesOf } from './savedTourStages'; import { tourPlannedTimes } from '../tourPresentation'; +import { savedStagePoints, savedTourStagesOf } from './savedTourStages'; import { StageDayPicker } from './StageDayPicker'; -import { stageNumberOf, TourStageDayPills } from './TourStageDayPills'; import { TourStageDayNotice } from './TourStageDayNotice'; +import { stageNumberOf, TourStageDayPills } from './TourStageDayPills'; import { StageTotalsRow, TourStageOverview } from './TourStageOverview'; import type { TourStageDays } from './useTourStageDays'; @@ -143,7 +143,12 @@ describe('the unfolded tour in the list', () => { render( ); const totals = screen.getByTestId('tour-stage-totals'); @@ -190,7 +195,11 @@ describe('the unfolded tour in the list', () => { const onToggle = vi.fn(); render(
    - +
@@ -272,7 +281,14 @@ describe('the stage a day holds', () => { const place = buildPlace({ id: TOUR.place_id, name: 'Hut to hut' }); const times = { ...tourPlannedTimes(TOUR), tour: TOUR }; const row = (tourStageKey?: string | null) => ( - + ); const view = render(row('stop-start')); expect(screen.getByTestId('trip-plan-tour-stage')).toHaveTextContent('Stage 1'); @@ -281,4 +297,31 @@ describe('the stage a day holds', () => { view.rerender(row('stop-gone')); expect(screen.queryByTestId('trip-plan-tour-stage')).toBeNull(); }); + + it('FE-TOUR-STAGE-OVERVIEW-016: the list and every day holding a stage of the tour share one read of it', async () => { + seed(); + render( + <> + + + + + ); + expect(await screen.findAllByTestId('tour-stage-item')).toHaveLength(2); + await waitFor(() => { + for (const pills of screen.getAllByTestId('trip-plan-tour-stage')) expect(pills).toHaveTextContent('km'); + }); + expect(tourRepo.detail).toHaveBeenCalledTimes(1); + }); + + it('FE-TOUR-STAGE-OVERVIEW-017: a read no row shows any more is let go, and the next row reads again', async () => { + seed(); + const first = render(); + const signal = vi.mocked(tourRepo.detail).mock.calls[0]?.[2]; + first.unmount(); + expect(signal?.aborted).toBe(true); + render(); + await waitFor(() => expect(screen.getByTestId('trip-plan-tour-stage')).toHaveTextContent('km')); + expect(tourRepo.detail).toHaveBeenCalledTimes(2); + }); }); diff --git a/client/src/components/Tours/stages/useSavedTourStages.ts b/client/src/components/Tours/stages/useSavedTourStages.ts index d0bfe3997..e2ee6b3fc 100644 --- a/client/src/components/Tours/stages/useSavedTourStages.ts +++ b/client/src/components/Tours/stages/useSavedTourStages.ts @@ -1,6 +1,6 @@ import type { TourListItem, TourWaypoint } from '@trek/shared'; import { useEffect, useMemo, useState } from 'react'; -import { tourRepo } from '../../../repo/tourRepo'; +import { tourPointsRepo } from '../../../repo/tourPointsRepo'; import { useTripStore } from '../../../store/tripStore'; import type { TourPlannerWaypoint } from '../planner/tourPlannerWaypoints'; import { savedStagePoints, savedTourStagesOf, type SavedTourStage } from './savedTourStages'; @@ -24,26 +24,37 @@ const NONE: SavedTourStages = { status: 'idle', stages: [], totals: null, points * the cache offline, and are read again once the tour's revision moves on, so * a saved change to the route shows here at once; the line is the trip's own * copy of the place, which a change updates too. Nothing is read while - * `enabled` is off (a folded row) or the tour has no stages. + * `enabled` is off (a folded row) or the tour has no stages, and the rows + * showing the same tour at once share one read. */ -export function useSavedTourStages(tripId: number | string | null | undefined, tour: TourListItem, enabled: boolean): SavedTourStages { - const routeGeometry = useTripStore((state) => state.places.find((place) => place.id === tour.place_id)?.route_geometry); +export function useSavedTourStages( + tripId: number | string | null | undefined, + tour: TourListItem, + enabled: boolean +): SavedTourStages { + const routeGeometry = useTripStore( + (state) => state.places.find((place) => place.id === tour.place_id)?.route_geometry + ); const revision = `${tour.place_id}:${tour.revision ?? 0}`; const wanted = enabled && tripId != null && (tour.stage_count ?? 0) > 1; const [loaded, setLoaded] = useState(null); useEffect(() => { if (!wanted || tripId == null) return undefined; - const controller = new AbortController(); - tourRepo - .detail(tripId, tour.place_id, controller.signal) - .then((detail) => { - if (!controller.signal.aborted) setLoaded({ revision, points: detail.waypoints }); - }) - .catch(() => { - if (!controller.signal.aborted) setLoaded({ revision, failed: true }); - }); - return () => controller.abort(); + const held = tourPointsRepo.hold(tripId, tour.place_id, revision); + let live = true; + held.points.then( + (points) => { + if (live) setLoaded({ revision, points }); + }, + () => { + if (live) setLoaded({ revision, failed: true }); + } + ); + return () => { + live = false; + held.release(); + }; }, [revision, tour.place_id, tripId, wanted]); return useMemo(() => { diff --git a/client/src/components/Tours/stages/useTourStageRail.test.ts b/client/src/components/Tours/stages/useTourStageRail.test.ts index 05e055137..5ac67ec01 100644 --- a/client/src/components/Tours/stages/useTourStageRail.test.ts +++ b/client/src/components/Tours/stages/useTourStageRail.test.ts @@ -1,7 +1,7 @@ -// FE-TOUR-STAGE-RAIL-001 to FE-TOUR-STAGE-RAIL-005: unfolding a multi-day tour in the planner's list and +// FE-TOUR-STAGE-RAIL-001 to FE-TOUR-STAGE-RAIL-007: unfolding a multi-day tour in the planner's list and // opening one of its stages there (MDT-02). -import type { TourListItem } from '@trek/shared'; import { act, renderHook } from '@testing-library/react'; +import type { TourListItem } from '@trek/shared'; import { describe, expect, it, vi } from 'vitest'; import type { TourPlannerController } from '../planner/useTourPlanner'; import type { TourStage } from './tourStages'; @@ -92,4 +92,34 @@ describe('useTourStageRail', () => { const dayTour = renderHook(() => useSavedTourStages(9, { ...TOUR, stage_count: null }, true)); expect(dayTour.result.current.status).toBe('idle'); }); + + it('FE-TOUR-STAGE-RAIL-006: an open the planner does not carry out lets the stage go, so a later open shows none', () => { + const openTour = vi.fn(); + let planner = plannerOf({ type: 'edit-saved', placeId: 5 }, false); + const { result, rerender } = renderHook(() => useTourStageRail(planner, openTour)); + act(() => result.current.openStage(TOUR, 'stop-hut')); + expect(openTour).toHaveBeenCalledWith(TOUR); + // The unsaved changes are kept: the planner stays on the tour it had open. + act(() => result.current.cancel()); + planner = plannerOf({ type: 'edit-saved', placeId: 81 }); + rerender(); + expect(planner.setSelectedWaypointId).not.toHaveBeenCalled(); + expect(planner.setRouteProfileFocus).not.toHaveBeenCalled(); + }); + + it('FE-TOUR-STAGE-RAIL-007: a stage the opened tour no longer has is let go once its points are in', () => { + let planner = plannerOf({ type: 'edit-saved', placeId: 81 }); + const { result, rerender } = renderHook(() => useTourStageRail(planner, vi.fn())); + act(() => result.current.openStage(TOUR, 'stop-gone')); + expect(planner.setSelectedWaypointId).not.toHaveBeenCalled(); + // The stop comes back later as a stage start: what was asked for before is not shown then. + const later = plannerOf({ type: 'edit-saved', placeId: 81 }); + planner = { + ...later, + waypoints: [...later.waypoints, { id: 'p4', key: 'stop-gone' }], + stages: { stages: [...later.stages.stages, { ...SECOND, number: 3, fromId: 'p4', toId: 'p3' }] }, + } as unknown as TourPlannerController; + rerender(); + expect(planner.setSelectedWaypointId).not.toHaveBeenCalled(); + }); }); diff --git a/client/src/components/Tours/stages/useTourStageRail.ts b/client/src/components/Tours/stages/useTourStageRail.ts index 22288ea7d..0076423b4 100644 --- a/client/src/components/Tours/stages/useTourStageRail.ts +++ b/client/src/components/Tours/stages/useTourStageRail.ts @@ -24,7 +24,10 @@ const openPlaceId = (planner: Pick) => * first when the open draft has unsaved changes), and once its points are in, * the stage is shown as the planner's own list of stages shows it. A stage of * the tour already open is shown at once. Asking for another tour, or opening - * another one, lets the stage go. + * another one, lets the stage go, and so does an open the planner does not + * carry out (`cancel`: the unsaved changes kept, the tour failing to open), so + * a later open of that tour from its row does not show a stage nobody asked + * for then. A stage the opened tour no longer has is let go as well. */ export function useTourStageRail(planner: RailPlanner, openTour: (tour: TourListItem) => void) { const [unfolded, setUnfolded] = useState>(() => new Set()); @@ -47,6 +50,7 @@ export function useTourStageRail(planner: RailPlanner, openTour: (tour: TourList }, [openTour, planner] ); + const cancel = useCallback(() => setPending(null), []); const open = openPlaceId(planner); const { waypoints } = planner; @@ -60,10 +64,14 @@ export function useTourStageRail(planner: RailPlanner, openTour: (tour: TourList } const start = waypoints.find((point) => point.key === pending.key); const stage = start && stages.find((item) => item.fromId === start.id); - if (!stage) return; + if (!stage) { + // The tour's points are in and no stage starts at that stop: the list it came from was stale. + if (waypoints.length > 0) setPending(null); + return; + } showStage(planner, stage); setPending(null); }, [open, pending, planner, stages, waypoints]); - return { isUnfolded, toggle, openStage }; + return { isUnfolded, toggle, openStage, cancel }; } diff --git a/client/src/repo/tourPointsRepo.test.ts b/client/src/repo/tourPointsRepo.test.ts new file mode 100644 index 000000000..8db7f43c5 --- /dev/null +++ b/client/src/repo/tourPointsRepo.test.ts @@ -0,0 +1,66 @@ +// FE-TOUR-POINTSREPO-001 to FE-TOUR-POINTSREPO-004: one read of a tour's saved points, shared by every +// view showing it at the same time (MDT-02). +import type { TourListItem, TourWaypoint } from '@trek/shared'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { tourPointsRepo } from './tourPointsRepo'; +import { tourRepo } from './tourRepo'; + +const TOUR = { place_id: 81, name: 'Hut to hut', tour_type: 'hike' } as TourListItem; +const POINTS: TourWaypoint[] = [ + { lat: 47, lng: 11, role: 'start', sequence: 0, key: 'stop-start' }, + { lat: 47.03, lng: 11, role: 'end', sequence: 1, key: 'stop-end' }, +]; + +beforeEach(() => { + vi.restoreAllMocks(); + vi.spyOn(tourRepo, 'detail').mockResolvedValue({ tour: TOUR, waypoints: POINTS }); +}); + +describe('tourPointsRepo', () => { + it('FE-TOUR-POINTSREPO-001: views showing the same tour at once share one read', async () => { + const first = tourPointsRepo.hold(9, 81, '81:2'); + const second = tourPointsRepo.hold(9, 81, '81:2'); + expect(await first.points).toEqual(POINTS); + expect(await second.points).toEqual(POINTS); + expect(tourRepo.detail).toHaveBeenCalledTimes(1); + expect(tourRepo.detail).toHaveBeenCalledWith(9, 81, expect.any(AbortSignal)); + first.release(); + second.release(); + }); + + it('FE-TOUR-POINTSREPO-002: a new revision is a read of its own', async () => { + const before = tourPointsRepo.hold(9, 81, '81:2'); + const after = tourPointsRepo.hold(9, 81, '81:3'); + await Promise.all([before.points, after.points]); + expect(tourRepo.detail).toHaveBeenCalledTimes(2); + before.release(); + after.release(); + }); + + it('FE-TOUR-POINTSREPO-003: the read is aborted once nobody holds it, and the next view reads again', async () => { + const first = tourPointsRepo.hold(9, 81, '81:2'); + const second = tourPointsRepo.hold(9, 81, '81:2'); + const signal = vi.mocked(tourRepo.detail).mock.calls[0]?.[2]; + first.release(); + // Letting go twice does not let go of the other view's hold. + first.release(); + expect(signal?.aborted).toBe(false); + second.release(); + expect(signal?.aborted).toBe(true); + const next = tourPointsRepo.hold(9, 81, '81:2'); + await next.points; + expect(tourRepo.detail).toHaveBeenCalledTimes(2); + next.release(); + }); + + it('FE-TOUR-POINTSREPO-004: a failed read is dropped at once, so the next view reads again', async () => { + vi.mocked(tourRepo.detail).mockRejectedValueOnce(new Error('offline')); + const failed = tourPointsRepo.hold(9, 81, '81:2'); + await expect(failed.points).rejects.toThrow('offline'); + const retry = tourPointsRepo.hold(9, 81, '81:2'); + expect(await retry.points).toEqual(POINTS); + expect(tourRepo.detail).toHaveBeenCalledTimes(2); + failed.release(); + retry.release(); + }); +}); diff --git a/client/src/repo/tourPointsRepo.ts b/client/src/repo/tourPointsRepo.ts new file mode 100644 index 000000000..2f6aa84f1 --- /dev/null +++ b/client/src/repo/tourPointsRepo.ts @@ -0,0 +1,52 @@ +import type { TourWaypoint } from '@trek/shared'; +import { tourRepo } from './tourRepo'; + +/** One read of a tour's saved points, shared by everyone showing that tour at that revision. */ +interface SharedRead { + points: Promise; + controller: AbortController; + holders: number; +} + +const reads = new Map(); + +/** + * MDT-02: the saved points of a tour, read once however many views show it at + * the same time: the fold in the planner's list and every day of the plan + * holding one of its stages would otherwise each ask for the same tour. + */ +export const tourPointsRepo = { + /** + * The points of the tour at this revision, through the tour repo (the cache + * standing in offline). A view holds the read while it shows the tour and + * releases it when it stops; once nobody holds it, it is dropped and, while + * still on its way, aborted. A failed read is dropped at once, so the next + * view to ask reads again, and a new revision is a read of its own. + */ + hold(tripId: number | string, placeId: number, revision: string) { + const key = `${tripId}:${placeId}:${revision}`; + const read = reads.get(key) ?? start(key, tripId, placeId); + read.holders += 1; + let held = true; + const release = () => { + if (!held) return; + held = false; + read.holders -= 1; + if (read.holders > 0) return; + if (reads.get(key) === read) reads.delete(key); + read.controller.abort(); + }; + return { points: read.points, release }; + }, +}; + +function start(key: string, tripId: number | string, placeId: number): SharedRead { + const controller = new AbortController(); + const points = tourRepo.detail(tripId, placeId, controller.signal).then((detail) => detail.waypoints); + const read: SharedRead = { points, controller, holders: 0 }; + points.catch(() => { + if (reads.get(key) === read) reads.delete(key); + }); + reads.set(key, read); + return read; +}