fix(tours): let an unwanted stage go and read a tour's stages once

Two review findings of MDT-02 in the planner's list and the day plan:

- A stage asked for while the draft has unsaved changes stayed pending when
  the changes were kept or the tour failed to open, so opening that tour
  later from its row jumped to a stage nobody asked for then. The rail now
  has cancel(), which the discard question calls when it is dismissed and
  the open calls when it fails, and it lets a stage go once the opened
  tour's points are in and none of its stages starts at that stop.
- Every day holding a stage of a tour read the whole tour on its own, so a
  five stage tour sent five identical requests on top of the list's own.
  tourPointsRepo shares one read per trip, tour and revision among the
  views showing it at the same time, through the tour repo as before, and
  drops it once the last view lets go or the read fails.
This commit is contained in:
Maurice
2026-10-20 15:04:04 +02:00
parent 3748bdb815
commit f57a0dbd31
8 changed files with 310 additions and 29 deletions
@@ -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 }) => (
<div data-testid="paddle-check-card" data-status={checklist.status} />
@@ -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<TourPlannerController>)
const dirty = planner({ mode: { type: 'edit-saved', placeId: 42 }, editingPlaceId: 42, hasUnsavedChanges: true })
const rail = (controller: TourPlannerController) => <TourPlannerToursRail planner={controller} tours={[tour, hutToHut]} {...railProps} />
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(<TourPlannerToursRail planner={planner({
mode: { type: 'new-draft' }, durationSeconds: 4800,
@@ -759,6 +759,8 @@ export function TourPlannerToursRail({ planner, tours, days, loading, onAssignTo
const distanceUnit = useSettingsStore(state => state.settings.distance_unit)
const tripId = useTripStore(state => state.trip?.id)
const [pendingTour, setPendingTour] = useState<TourListItem | null>(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
<ConfirmDialog
isOpen={pendingTour !== null}
onClose={() => 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)
}}
@@ -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(
<StageTotalsRow
tour={TOUR}
totals={{ distanceKm: { value: 10, known: 1, of: 2 }, gain: unknown, loss: unknown, minutes: { value: 300, known: 2, of: 2 } }}
totals={{
distanceKm: { value: 10, known: 1, of: 2 },
gain: unknown,
loss: unknown,
minutes: { value: 300, known: 2, of: 2 },
}}
/>
);
const totals = screen.getByTestId('tour-stage-totals');
@@ -190,7 +195,11 @@ describe('the unfolded tour in the list', () => {
const onToggle = vi.fn();
render(
<ul>
<TourListRow tour={TOUR} onSelect={onSelect} fold={{ expanded: true, onToggle, label: 'Hide stages', controls: 'stages' }}>
<TourListRow
tour={TOUR}
onSelect={onSelect}
fold={{ expanded: true, onToggle, label: 'Hide stages', controls: 'stages' }}
>
<TourStageOverview id="stages" tripId={9} tour={TOUR} days={DAYS} canAssign />
</TourListRow>
</ul>
@@ -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) => (
<DayPlanPlaceSubline place={place} cat={undefined} tourTimes={times} isTourPlace timeFormat="24h" tourStageKey={tourStageKey} />
<DayPlanPlaceSubline
place={place}
cat={undefined}
tourTimes={times}
isTourPlace
timeFormat="24h"
tourStageKey={tourStageKey}
/>
);
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(
<>
<TourStageOverview id="stages" tripId={9} tour={TOUR} days={DAYS} canAssign />
<TourStageDayPills tour={TOUR} stageKey="stop-start" />
<TourStageDayPills tour={TOUR} stageKey="stop-hut" />
</>
);
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(<TourStageDayPills tour={TOUR} stageKey="stop-hut" />);
const signal = vi.mocked(tourRepo.detail).mock.calls[0]?.[2];
first.unmount();
expect(signal?.aborted).toBe(true);
render(<TourStageDayPills tour={TOUR} stageKey="stop-hut" />);
await waitFor(() => expect(screen.getByTestId('trip-plan-tour-stage')).toHaveTextContent('km'));
expect(tourRepo.detail).toHaveBeenCalledTimes(2);
});
});
@@ -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<Loaded | null>(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(() => {
@@ -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();
});
});
@@ -24,7 +24,10 @@ const openPlaceId = (planner: Pick<TourPlannerController, 'mode'>) =>
* 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<ReadonlySet<number>>(() => 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 };
}
+66
View File
@@ -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();
});
});
+52
View File
@@ -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<readonly TourWaypoint[]>;
controller: AbortController;
holders: number;
}
const reads = new Map<string, SharedRead>();
/**
* 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;
}