diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index 962c1bb4f..72005e7c4 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -711,10 +711,23 @@ export function MapViewGL({ // sits on that surface. Stopping only pointerdown left both running at once: the // handle moved AND the map slid out from under it. const swallow = (e: Event) => e.stopPropagation() + // Where the gesture began, so a click can be told from a drag. Without it + // every pointerup committed a move to wherever inside the 17px handle the + // pointer happened to be: at a low zoom that is kilometres, and the write + // re-routes the whole trip. Leaflet gets this for free from `dragend`, + // which is why the two renderers behaved differently for one gesture. + let startedAt: { x: number; y: number } | null = null + const MOVED_ENOUGH = 4 const onDown = (e: PointerEvent) => { + // Primary button only. Right-clicking ran onDown, then contextmenu -> + // remove AND pointerup -> move, so one gesture sent two writes; on the + // platforms where contextmenu comes first, the move landed on an id that + // no longer existed and reported a failure for a deletion that worked. + if (e.button !== 0) return e.stopPropagation() e.preventDefault() dragging = true + startedAt = { x: e.clientX, y: e.clientY } el.setPointerCapture(e.pointerId) el.style.cursor = 'grabbing' } @@ -728,10 +741,29 @@ export function MapViewGL({ if (!dragging) return dragging = false el.style.cursor = 'grab' + const from = startedAt + startedAt = null + const travelled = from ? Math.hypot(e.clientX - from.x, e.clientY - from.y) : 0 + if (travelled < MOVED_ENOUGH) { + // A click, not a drag. Put the pin back where the via actually is: the + // move handler has been following the pointer across the handle. + pin.setLngLat([via.lng, via.lat]) + return + } const rect = map.getContainer().getBoundingClientRect() const at = map.unproject([e.clientX - rect.left, e.clientY - rect.top]) viaHandlersRef.current.onMoveVia?.(via.day_id, via.id, at.lat, at.lng) } + // The browser fires this when the captured element is torn out of the DOM, + // which is exactly what a delete does. Committing a position there wrote a + // move for a via that was being removed. + const onCancel = () => { + if (!dragging) return + dragging = false + startedAt = null + el.style.cursor = 'grab' + pin.setLngLat([via.lng, via.lat]) + } const onContext = (e: MouseEvent) => { e.preventDefault() viaHandlersRef.current.onRemoveVia?.(via.day_id, via.id) @@ -739,7 +771,7 @@ export function MapViewGL({ el.addEventListener('pointerdown', onDown) el.addEventListener('pointermove', onMove) el.addEventListener('pointerup', onUp) - el.addEventListener('pointercancel', onUp) + el.addEventListener('pointercancel', onCancel) el.addEventListener('contextmenu', onContext) el.addEventListener('mousedown', swallow) el.addEventListener('touchstart', swallow, { passive: true }) @@ -748,7 +780,7 @@ export function MapViewGL({ el.removeEventListener('pointerdown', onDown) el.removeEventListener('pointermove', onMove) el.removeEventListener('pointerup', onUp) - el.removeEventListener('pointercancel', onUp) + el.removeEventListener('pointercancel', onCancel) el.removeEventListener('contextmenu', onContext) el.removeEventListener('mousedown', swallow) el.removeEventListener('touchstart', swallow) @@ -870,8 +902,17 @@ export function MapViewGL({ useEffect(() => { const map = mapRef.current if (!map || !mapReady || !onRouteClick) return - const onClick = (e: { lngLat: { lat: number; lng: number }; preventDefault?: () => void }) => { + const onClick = (e: { lngLat: { lat: number; lng: number }; point?: { x: number; y: number }; preventDefault?: () => void }) => { e.preventDefault?.() + // Layer handlers are independent: MapLibre evaluates each registration + // against the same click, so a point that hits both the offered-route band + // and the current-route band fires both. Every alternative starts and ends + // at the same two stops, and when the leg already carries vias the current + // road is itself one of the offers, so the overlap is the whole leg. One + // click then dropped a via AND chose a different road, two writes and two + // trip reloads for a gesture the user made once. The picker is a modal + // choice about that leg; dropping a via mid-choice is not offered anywhere. + if (e.point && map.queryRenderedFeatures?.(e.point, { layers: ['route-alt-hit'] })?.length) return onRouteClick(e.lngLat.lat, e.lngLat.lng) } const enter = () => { map.getCanvas().style.cursor = 'copy' } diff --git a/client/src/components/Roadtrip/RoadtripCategoryPicker.tsx b/client/src/components/Roadtrip/RoadtripCategoryPicker.tsx index 11ad6b421..6fd0d95f0 100644 --- a/client/src/components/Roadtrip/RoadtripCategoryPicker.tsx +++ b/client/src/components/Roadtrip/RoadtripCategoryPicker.tsx @@ -66,7 +66,7 @@ export default function RoadtripCategoryPicker({ keys, meta, selected, onToggle key={k} className="grid h-[18px] w-[18px] place-items-center rounded-md" // theme-lint-disable — the road-signage palette in `roadtripModel`. - style={{ background: serviceColor(k), color: '#fff' }} + style={{ background: serviceColor(k), color: '#fff' }} // theme-lint-disable — road-signage palette > @@ -105,8 +105,7 @@ export default function RoadtripCategoryPicker({ keys, meta, selected, onToggle > diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx index c9dd9b830..be56236b4 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.test.tsx @@ -31,7 +31,10 @@ const day = (dayId: number, dayNumber: number): RoadtripDay => ({ legs: [], schedule: { entries: [], warnings: [] }, legVias: [], driveWarnings: [], dayWarning: null, - geometry: [], + // Routed. Searching before the day has a geometry builds the corridor from the + // straight line between the stops, and the routing answer landing a second + // later clears the search mid-flight — so the button waits for this. + geometry: [[53.55, 9.99], [52.52, 13.4]] as [number, number][], distance: 0, duration: 0, }) @@ -120,6 +123,27 @@ describe('RoadtripCorridorPanel', () => { expect(c.search.search).toHaveBeenCalledTimes(1) }) + it('FE-ROADTRIP-PANEL-004: a day that has not routed yet cannot be searched along', () => { + // Until the day routes, the corridor is the straight line between the stops, + // so the boxes march across whatever lies between them instead of along the + // roads driven. Worse, the routing answer landing a second later changes the + // line, which clears the search mid-flight and drops every result with no + // error and no explanation. Routing runs one day at a time, about a second + // apart, so on a long trip that window is wide open. + const unrouted = { ...day(1, 1), geometry: [] as [number, number][] } + const c = corridor({ day: unrouted }) + wrap() + + expect(screen.getByRole('button', { name: /search/i })).toBeDisabled() + }) + + it('FE-ROADTRIP-PANEL-005: nor while the routing is still running', () => { + const c = corridor() + wrap() + + expect(screen.getByRole('button', { name: /search/i })).toBeDisabled() + }) + it('FE-ROADTRIP-PANEL-002: nothing selected means nothing to search for', () => { const c = corridor({ categories: [] }) wrap() diff --git a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx index 222e05033..3d1beffba 100644 --- a/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx +++ b/client/src/components/Roadtrip/RoadtripCorridorPanel.tsx @@ -173,7 +173,7 @@ function ResultGroup({ category, pois, dayId, insertIndexFor, onAddPoi }: { @@ -244,7 +244,19 @@ export default function RoadtripCorridorPanel({ corridor, routes, onAddPoi }: Ro const filteredToNothing = search.results.length > 0 && corridor.visible.length === 0 - const canSearch = !search.loading && corridor.categories.length > 0 && (corridor.day?.stops.length ?? 0) > 1 + // The day has to have routed. Until it does, the corridor is built from the + // straight line between the stops, so the boxes march across whatever lies + // between them rather than along the roads actually driven — and when the + // routing answer lands a second later the line changes, which clears the + // search mid-flight and drops every result with no error and no explanation. + // The routing runs one day at a time, about a second apart, so on a long trip + // that window is wide open for the day somebody picks. + const dayRouted = (corridor.day?.geometry.length ?? 0) > 1 + const canSearch = !search.loading + && !routes.loading + && dayRouted + && corridor.categories.length > 0 + && (corridor.day?.stops.length ?? 0) > 1 const progressPct = search.progress.total ? Math.round((search.progress.done / search.progress.total) * 100) : 0 diff --git a/client/src/components/Roadtrip/RoadtripSidebar.tsx b/client/src/components/Roadtrip/RoadtripSidebar.tsx index 2803e31e8..5c2e5f0fc 100644 --- a/client/src/components/Roadtrip/RoadtripSidebar.tsx +++ b/client/src/components/Roadtrip/RoadtripSidebar.tsx @@ -416,7 +416,7 @@ function ServiceStop({ stop, entry, driveFindings, selected, onSelect, onEditSta // theme-lint-disable — the road-signage palette in `roadtripModel`, shared // with the corridor list and the map pin so one kind of stop looks like // itself wherever it turns up. - style={{ background: serviceColor(stop.stopType), color: '#fff' }} + style={{ background: serviceColor(stop.stopType), color: '#fff' }} // theme-lint-disable — road-signage palette > @@ -425,7 +425,7 @@ function ServiceStop({ stop, entry, driveFindings, selected, onSelect, onEditSta diff --git a/client/src/components/Roadtrip/StopKindPicker.tsx b/client/src/components/Roadtrip/StopKindPicker.tsx index 1d8615235..11dfbf4c2 100644 --- a/client/src/components/Roadtrip/StopKindPicker.tsx +++ b/client/src/components/Roadtrip/StopKindPicker.tsx @@ -105,7 +105,7 @@ export default function StopKindPicker({ anchor, current, onPick, onClose }: { // theme-lint-disable — the road-signage palette from `roadtripModel`, the // same one the rail disc and the map pin use, so the choice looks like // what it will become. - style={{ background: color, color: '#fff', ...(on ? { boxShadow: `0 0 0 2px ${color}` } : {}) }} + style={{ background: color, color: '#fff', ...(on ? { boxShadow: `0 0 0 2px ${color}` } : {}) }} // theme-lint-disable — road-signage palette > diff --git a/client/src/components/Roadtrip/roadtripModel.test.ts b/client/src/components/Roadtrip/roadtripModel.test.ts index f3e971b48..0b7ea966a 100644 --- a/client/src/components/Roadtrip/roadtripModel.test.ts +++ b/client/src/components/Roadtrip/roadtripModel.test.ts @@ -490,3 +490,58 @@ describe('sectionAnchors', () => { expect(sectionAnchors([])).toEqual([]) }) }) + +describe('deriveDriveWarnings — what counts as driving', () => { + const drive = (minutes: number, km: number) => + ({ duration: minutes * 60, distance: km * 1000, mode: 'driving' }) + const onFoot = (minutes: number, km: number) => + ({ duration: minutes * 60, distance: km * 1000, mode: 'walking' }) + + it('FE-ROADTRIP-MODEL-085: a walk to a viewpoint is not over the longest drive allowed', () => { + // A day may legitimately mix modes — splitIntoRuns exists for exactly that — + // and a two hour hike reported as a leg finding reads as a fault in a plan + // that has none. + const out = deriveDriveWarnings( + [drive(60, 80), onFoot(120, 6)], + [false, false, false], + { legMinutes: 90, dayMinutes: null, rangeKm: null }, + 0, + ) + expect(out.warnings).toEqual([]) + }) + + it('FE-ROADTRIP-MODEL-086: walked kilometres are not on the tank', () => { + // Nor does walking fill it: the budget is left exactly as the drive left it. + const out = deriveDriveWarnings( + [drive(60, 80), onFoot(120, 60), drive(30, 30)], + [false, false, false, false], + { legMinutes: null, dayMinutes: null, rangeKm: 100 }, + 0, + ) + // 80 then 110 — the 60 walked kilometres never entered the budget. + expect(out.warnings).toEqual([{ index: 3, code: 'range', sinceKm: 110 }]) + expect(out.carryKm).toBe(110) + }) + + it('FE-ROADTRIP-MODEL-087: a walk does not count toward the driving time of the day', () => { + const out = deriveDriveWarnings( + [drive(60, 80), onFoot(180, 9)], + [false, false, false], + { legMinutes: null, dayMinutes: 90, rangeKm: null }, + 0, + ) + expect(out.day).toBeNull() + }) + + it('FE-ROADTRIP-MODEL-088: a leg with no mode is still a drive', () => { + // Every leg was a drive before the field existed, so an absent mode counts. + // Reading it the other way round would silently switch the warnings off. + const out = deriveDriveWarnings( + [{ duration: 120 * 60, distance: 200 * 1000 }], + [false, false], + { legMinutes: 90, dayMinutes: null, rangeKm: null }, + 0, + ) + expect(out.warnings).toEqual([{ index: 1, code: 'leg', overMinutes: 30 }]) + }) +}) diff --git a/client/src/components/Roadtrip/roadtripModel.ts b/client/src/components/Roadtrip/roadtripModel.ts index 9af26099b..c61a8cefd 100644 --- a/client/src/components/Roadtrip/roadtripModel.ts +++ b/client/src/components/Roadtrip/roadtripModel.ts @@ -329,7 +329,16 @@ export function sumLegSeconds(legSeconds: (number | undefined)[]): number { * schedule abandons its cursor: a distance we do not have cannot be added to one we do. */ export function deriveDriveWarnings( - legs: ({ duration?: number; distance?: number } | undefined)[], + /** + * `mode` decides whether a leg counts at all. A day may legitimately mix + * modes — splitIntoRuns exists for exactly that — and a two hour walk to a + * viewpoint is not two hours of driving, nor are its kilometres on the tank. + * Counting them reported a hike as "over the longest drive allowed" and let a + * ferry crossing trip a range warning. Anything that is not a driving profile + * is skipped; an absent mode counts, because every leg was a drive before the + * field existed. + */ + legs: ({ duration?: number; distance?: number; mode?: string } | undefined)[], /** Whether the stop at each index refuels. One entry longer than `legs`. */ refuelsAt: boolean[], limits: DriveLimits, @@ -345,6 +354,11 @@ export function deriveDriveWarnings( const leg = legs[i] // The stop this leg arrives at. Both findings are about what is true on arrival. const at = i + 1 + // Not a drive: neither its minutes nor its kilometres belong in findings + // about the driving. The budget is left exactly as it was — walking to a + // viewpoint and back does not use fuel, and it does not fill the tank + // either. + if (leg && leg.mode !== undefined && leg.mode !== 'driving') continue const seconds = leg?.duration if (typeof seconds === 'number') { totalSeconds += seconds diff --git a/client/src/components/Roadtrip/useFollowTrack.test.ts b/client/src/components/Roadtrip/useFollowTrack.test.ts index 473633853..50989fd8d 100644 --- a/client/src/components/Roadtrip/useFollowTrack.test.ts +++ b/client/src/components/Roadtrip/useFollowTrack.test.ts @@ -49,6 +49,7 @@ function viasStub(over: Partial = {}): RoadtripVias { byDay: {}, trackByDay: {}, stale: false, + editable: true, add: vi.fn(), addMany: vi.fn().mockResolvedValue(undefined), move: vi.fn(), diff --git a/client/src/components/Roadtrip/useRoadtripVias.ts b/client/src/components/Roadtrip/useRoadtripVias.ts index 80cbf2561..4dc8c6f0c 100644 --- a/client/src/components/Roadtrip/useRoadtripVias.ts +++ b/client/src/components/Roadtrip/useRoadtripVias.ts @@ -1,8 +1,27 @@ import { useCallback, useEffect, useState } from 'react' import { roadtripApi } from '../../api/client' +import { useNetworkMode } from '../../hooks/useNetworkMode' import { isEmptyReanchoring, type Reanchoring } from './roadtripModel' import type { RoadtripDayTrack, RoadtripVia } from '@trek/shared' +/** + * Via points are DELIBERATELY online-only, which the repo asks to be said out + * loud rather than left to be discovered. + * + * Everything else on this screen goes through a repo and the mutation queue: + * optimistic Dexie write, temporary negative id, idempotent replay on + * reconnect. A via cannot follow that shape as it stands, because its anchor is + * a POSITION rather than an id — `after_order_index` counts stops as the routing + * request builds them — so a queued write replayed after somebody else has + * added or removed a stop would re-pin the drive onto a leg nobody chose, and it + * would do so silently, hours later. The correct offline story needs the anchor + * to be an assignment id first, which is a change to the wire contract and to + * the migration, not to this hook. + * + * Until then the honest behaviour is to say so: `editable` is false with no + * network, so the map stops offering a drag it cannot keep, instead of failing + * one write at a time and losing the edit. + */ export interface RoadtripVias { /** Every via of the trip, keyed by day. */ byDay: Record @@ -24,6 +43,14 @@ export interface RoadtripVias { * them. The list is kept and the staleness is said instead. */ stale: boolean + /** + * Whether shaping the drive is possible right now. + * + * False with no network: the domain is online-only (see the note above this + * interface), so the surfaces disable the gesture rather than accepting one + * and dropping it. + */ + editable: boolean add: (dayId: number, afterOrderIndex: number, lat: number, lng: number) => Promise /** * Lay a chain of vias on one day, optionally clearing the legs it fills first. @@ -76,6 +103,9 @@ export function useRoadtripVias(tripId: number | string | null, active: boolean) * quietly showing a trip that has lost its detours. */ const [stale, setStale] = useState(false) + // Read through the app's single source of truth for connectivity, which also + // covers the offline switch a user can set with the network still up. + const { offline } = useNetworkMode() const group = useCallback((vias: RoadtripVia[]) => { const next: Record = {} @@ -159,5 +189,5 @@ export function useRoadtripVias(tripId: number | string | null, active: boolean) await reload() }, [tripId, reload]) - return { byDay, trackByDay, stale, add, addMany, move, remove, reanchor } + return { byDay, trackByDay, stale, editable: !offline, add, addMany, move, remove, reanchor } } diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index e67ee4afa..02b26bb6e 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -1034,6 +1034,15 @@ export function useTripPlanner() { if (!routeAlternatives.open) setHighlightedAlternative(null) }, [routeAlternatives.open]) + // Leaving road trip mode closes it too. The switch sits in the left sidebar and + // is reachable while the bar is open over the map, and the overlay depends only + // on the picker — so flipping the mode off left pale blue alternatives, their + // casings and their drive-time pills drawn on an ordinary planner map, with no + // road trip UI left to dismiss them from. + useEffect(() => { + if (!roadtripActive) routeAlternatives.close() + }, [roadtripActive, routeAlternatives]) + /** * The offered routes as the map draws them: line, colour, and the label that sits on * the road. Built here rather than in the page so the page stays a wiring container @@ -1103,7 +1112,19 @@ export function useTripPlanner() { const dayIndex = stop && day ? day.stops.indexOf(stop) : -1 // Clearing the leg in one write. One delete per via meant a full trip re-route // between each of them, so undoing a detour with three vias drew three routes. - if (dayIndex >= 0) await roadtripVias.addMany(open.dayId, [], [dayIndex]).catch(() => {}) + // + // Reported like every other write in this hook. Swallowing it closed the + // picker on a leg that still carries its via and still routes the old way, + // so the traveller believed they had undone the detour — and because the + // request failed, not even the reload ran to contradict them. + if (dayIndex >= 0) { + try { + await roadtripVias.addMany(open.dayId, [], [dayIndex]) + } catch (err: unknown) { + toast.error(err instanceof Error ? err.message : t('common.unknownError')) + return + } + } routeAlternatives.close() return } diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index b9471660e..440f22f94 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -447,7 +447,8 @@ describe('Tool: search_pois', () => { expect(data.pois[0].name).toBe('Chez Nous'); expect(data.source).toBe('openstreetmap'); expect(trekNearbyMock).toHaveBeenCalled(); - expect(MapsService.prototype.searchOverpassPois).toHaveBeenCalledWith('restaurant', BBOX, 'fr'); + // The caller's per-category budget rides along on the fallback too. + expect(MapsService.prototype.searchOverpassPois).toHaveBeenCalledWith('restaurant', BBOX, 'fr', 60); }); }); @@ -497,7 +498,9 @@ describe('Tool: search_pois', () => { expect(lat).toBeCloseTo(48.86, 5); expect(lng).toBeCloseTo(2.345, 5); expect(opts?.category).toBe('restaurant,casual_eatery,fast_food'); - expect(opts?.limit).toBe(50); + // Sixty per category, the same allowance the Overpass path spends, rather + // than a number this branch made up for itself. + expect(opts?.limit).toBe(60); expect(opts?.radius).toBeGreaterThan(300); expect(opts?.radius).toBeLessThanOrEqual(20000); }); @@ -532,7 +535,7 @@ describe('Tool: search_pois', () => { await withHarness(user.id, async (h) => { await h.client.callTool({ name: 'search_pois', arguments: { category: 'museum', bbox: BBOX } }); - expect(MapsService.prototype.searchOverpassPois).toHaveBeenCalledWith('museum', BBOX, undefined); + expect(MapsService.prototype.searchOverpassPois).toHaveBeenCalledWith('museum', BBOX, undefined, 60); }); }); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index d148beef1..8aa032465 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -2698,7 +2698,9 @@ describe('controller-facing wrappers delegate to the folded methods', () => { // The index answers with an empty page (see the mock above), so what this // measures is the wrapper's own delegation. await svc.pois('cafe', bbox, 'de'); - expect(spies.searchOverpassPois).toHaveBeenCalledWith('cafe', bbox, 'de'); + // The caller's per-category budget rides along, so a corridor search that + // falls through to Overpass gets the allowance it asked the index for. + expect(spies.searchOverpassPois).toHaveBeenCalledWith('cafe', bbox, 'de', 60); } finally { Object.values(spies).forEach((s) => s.mockRestore()); } diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index 6b528703e..927b8dad1 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -587,10 +587,12 @@ describe('toPlaceRecord', () => { describe('POI_CATEGORY_TO_TREK', () => { it('covers every pill the planner offers', () => { // The keys are the contract with the client's POI_CATEGORIES. A missing one - // silently drops that pill back to Overpass, which is the slow path. + // silently drops that pill back to Overpass, which is the slow path — and + // for the four a drive needs that is the worst path of all, because the + // corridor search asks for them sixteen boxes at a time. expect(Object.keys(POI_CATEGORY_TO_TREK).sort()).toEqual([ - 'activity', 'bar', 'cafe', 'hotel', 'museum', - 'nature', 'restaurant', 'shopping', 'sights', 'supermarket', + 'activity', 'bar', 'cafe', 'campsite', 'charging', 'fuel', 'hotel', 'museum', + 'nature', 'rest_area', 'restaurant', 'shopping', 'sights', 'supermarket', ]); });