From 59b7c35e871b1e9ecdbc53dcaad44a61bf5e5494 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 9 Sep 2026 18:25:23 +0200 Subject: [PATCH] fix(roadtrip): re-pin a via to the leg it was dragged onto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dragging a via somewhere else on the map changed where the point IS and never where it sits in the chain. The anchor that decides which two stops it is threaded between was worked out once, when the point was dropped, and the drag sent only coordinates — `roadtripViaUpdateRequestSchema` carried a lat and a lng and nothing else. So a via pulled past the stop it used to precede kept claiming the earlier leg. The router then drove out to it and turned back before carrying on, which on screen is not a subtly wrong route: it is a spur that reads as the drag having done nothing at all, with the original road still sitting there unchanged. The anchor is not a property of the point but of where the point sits along the drive, so it is recomputed on a drag the same way it is on a drop. The projection that did that was inline in addRoadtripVia; it moves to a shared `anchorFor` and both callers use it. A drag differs from a drop in two ways, both deliberate. It measures against its OWN day: letting the nearest day win, which is right for a click on open map, would hand the via to a neighbouring day whose road happens to pass closer and make it disappear from the day it was dragged in. And it has no 2 km guard: pulling a via well off the current road is exactly what the gesture is for, and the anchor only says which leg gets bent. `after_order_index` is optional on the way in, so a drag inside one leg and an older client still send just the coordinates, and the stored pin is then left alone rather than cleared. --- .../components/Roadtrip/useRoadtripVias.ts | 13 +++-- .../useTripPlanner.roadtrip.test.tsx | 54 ++++++++++++++++++- .../src/pages/tripPlanner/useTripPlanner.ts | 33 ++++++++++-- .../src/nest/roadtrip/roadtrip.controller.ts | 2 +- server/src/nest/roadtrip/roadtrip.service.ts | 24 ++++++++- .../unit/nest/roadtrip.controller.test.ts | 10 +++- .../tests/unit/nest/roadtrip.service.test.ts | 25 ++++++++- shared/src/roadtrip/roadtrip.schema.ts | 12 ++++- 8 files changed, 158 insertions(+), 15 deletions(-) diff --git a/client/src/components/Roadtrip/useRoadtripVias.ts b/client/src/components/Roadtrip/useRoadtripVias.ts index 4dc8c6f0c..b995ac08d 100644 --- a/client/src/components/Roadtrip/useRoadtripVias.ts +++ b/client/src/components/Roadtrip/useRoadtripVias.ts @@ -66,7 +66,7 @@ export interface RoadtripVias { /** Absent leaves the day's track alone, null clears it, an object records a new one. */ track?: { place_id: number; stray_km?: number | null } | null, ) => Promise - move: (dayId: number, id: number, lat: number, lng: number) => Promise + move: (dayId: number, id: number, lat: number, lng: number, afterOrderIndex?: number) => Promise remove: (dayId: number, id: number) => Promise /** * Correct a day's anchors after its stops changed shape. @@ -171,9 +171,16 @@ export function useRoadtripVias(tripId: number | string | null, active: boolean) await reload() }, [tripId, reload]) - const move = useCallback(async (dayId: number, id: number, lat: number, lng: number) => { + const move = useCallback(async (dayId: number, id: number, lat: number, lng: number, afterOrderIndex?: number) => { if (!tripId) return - await roadtripApi.moveVia(tripId, dayId, id, { lat, lng }) + // The anchor rides along when the caller worked out a new one: a via dragged past the + // stop it used to sit before belongs to the next leg now, and saying only where it is + // leaves it claiming the old one. + await roadtripApi.moveVia(tripId, dayId, id, { + lat, + lng, + ...(afterOrderIndex === undefined ? {} : { after_order_index: afterOrderIndex }), + }) await reload() }, [tripId, reload]) diff --git a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx index e9df4a853..60ba14115 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.roadtrip.test.tsx @@ -1060,13 +1060,65 @@ describe('useTripPlanner road trip: shaping the drive on the map', () => { const { result } = await renderRoadtrip() await act(async () => { await result.current.moveRoadtripVia(5, 3, 53.2, 11.7) }) - expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 53.2, 11.7) + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 53.2, 11.7, 0) rt.vias.move.mockRejectedValue(new Error('stale')) await act(async () => { await result.current.moveRoadtripVia(5, 3, 53.2, 11.7) }) expect(toasts.some(t => t.type === 'error' && t.message === 'stale')).toBe(true) }) + it('FE-TP-ROAD-053: a via dragged past a stop is re-pinned to the leg it landed on', async () => { + // The bug this exists for. A drag used to send only the new coordinates, so a via + // pulled beyond the stop it used to precede kept claiming the earlier leg: the route + // ran out to the point and back before carrying on, which reads as the drag doing + // nothing at all. Three stops so there are two legs to land between. + seedTrip({ days: [buildDay({ id: 5, day_number: 1 })] }) + rt.corridor.day = { dayId: 5, dayNumber: 1 } + rt.routes.days = [{ + dayId: 5, + stops: [{ lat: 53.55, lng: 9.99 }, { lat: 53.85, lng: 11.45 }, { lat: 52.52, lng: 13.4 }], + geometry: [[53.55, 9.99], [53.87, 10.7], [53.85, 11.45], [53.87, 11.53], [52.52, 13.4]], + }] + const { result } = await renderRoadtrip() + + // Dropped on the first leg, then dragged past the middle stop onto the second. + await act(async () => { await result.current.addRoadtripVia(53.87, 10.7) }) + expect(rt.vias.add).toHaveBeenCalledWith(5, 0, 53.87, 10.7) + + await act(async () => { await result.current.moveRoadtripVia(5, 9, 53.87, 11.53) }) + expect(rt.vias.move).toHaveBeenCalledWith(5, 9, 53.87, 11.53, 1) + }) + + it('FE-TP-ROAD-054: a drag is measured against its own day, however close another one runs', async () => { + // Placing a via lets the nearest day win, which is right for a click on the map. + // A drag is not that: the via already belongs to a day, and handing it to a + // neighbouring day whose road happens to pass closer would make it vanish from the + // one it was dragged in. + twoRoutedDays() + const { result } = await renderRoadtrip() + + // Right on day 6's line, but dragged within day 5. + await act(async () => { await result.current.moveRoadtripVia(5, 3, 47.9, 12.3) }) + + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 47.9, 12.3, expect.any(Number)) + // Day 6 never saw it. + expect(rt.vias.move).not.toHaveBeenCalledWith(6, expect.anything(), expect.anything(), expect.anything(), expect.anything()) + }) + + it('FE-TP-ROAD-055: a drag far off the road still moves, without an anchor to offer', async () => { + // No distance guard on a drag, unlike on a click: pulling a via well away from the + // current road is the whole point of the gesture. A day with no line to measure + // against simply sends no anchor, and the existing pin stays as it was. + seedTrip({ days: [buildDay({ id: 5, day_number: 1 })] }) + rt.corridor.day = { dayId: 5, dayNumber: 1 } + rt.routes.days = [{ dayId: 5, stops: [], geometry: [] }] + const { result } = await renderRoadtrip() + + await act(async () => { await result.current.moveRoadtripVia(5, 3, 41.9, 12.5) }) + + expect(rt.vias.move).toHaveBeenCalledWith(5, 3, 41.9, 12.5, undefined) + }) + it('FE-TP-ROAD-052: removing a via lets the drive take the direct road again', async () => { twoRoutedDays() const { result } = await renderRoadtrip() diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index b180a6fc2..dce17731d 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -1148,10 +1148,19 @@ export function useTripPlanner() { * third stop" means the same thing everywhere. The day is the one whose line was hit, * found by trying each day's geometry and keeping the closest. */ - const addRoadtripVia = useCallback(async (lat: number, lng: number) => { - if (!can('day_edit', trip)) return + /** + * Which stop of which day a point belongs behind, measured along the drive. + * + * Shared by placing a via and by dragging one, because it is the same question both + * times and the answer has to be recomputed both times. A drag used to send only the + * new coordinates, so a via pulled past the stop it used to precede kept claiming the + * earlier leg: the route then ran out to the point and back before carrying on, which + * looks exactly like a drag that did nothing. + */ + const anchorFor = useCallback((lat: number, lng: number, onlyDayId?: number) => { let best: { dayId: number; afterIndex: number; offRouteKm: number } | null = null for (const day of roadtripRoutes.days) { + if (onlyDayId !== undefined && day.dayId !== onlyDayId) continue if (day.geometry.length < 2) continue const spine = day.geometry.map(([la, ln]) => ({ lat: la, lng: ln })) const hit = projectOntoRoute({ lat, lng }, spine) @@ -1162,6 +1171,12 @@ export function useTripPlanner() { const afterIndex = Math.max(0, insertIndexForAlong(stopsAlong, hit.alongKm) - 1) best = { dayId: day.dayId, afterIndex, offRouteKm: hit.offRouteKm } } + return best + }, [roadtripRoutes.days]) + + const addRoadtripVia = useCallback(async (lat: number, lng: number) => { + if (!can('day_edit', trip)) return + const best = anchorFor(lat, lng) // A click that landed on some other line is not a via anywhere. if (!best || best.offRouteKm > 2) return try { @@ -1169,17 +1184,25 @@ export function useTripPlanner() { } catch (err: unknown) { toast.error(err instanceof Error ? err.message : t('common.unknownError')) } - }, [roadtripRoutes.days, roadtripVias, can, trip, toast, t]) + }, [anchorFor, roadtripVias, can, trip, toast, t]) /** Dragging a via redraws the route through its new position. */ const moveRoadtripVia = useCallback(async (dayId: number, id: number, lat: number, lng: number) => { if (!can('day_edit', trip)) return + // Measured against this day only. A drag is a drag WITHIN a day: letting the nearest + // day win, the way placing one does, would hand the via to a neighbouring day whose + // road happens to pass closer, and it would vanish from the day it was dragged in. + // + // No distance guard either. Dragging a via well off the current road is the whole + // point of dragging it, and refusing that would be refusing the gesture; the anchor + // just says which leg gets bent, and the router answers the rest. + const anchor = anchorFor(lat, lng, dayId) try { - await roadtripVias.move(dayId, id, lat, lng) + await roadtripVias.move(dayId, id, lat, lng, anchor?.afterIndex) } catch (err: unknown) { toast.error(err instanceof Error ? err.message : t('common.unknownError')) } - }, [roadtripVias, can, trip, toast, t]) + }, [anchorFor, roadtripVias, can, trip, toast, t]) /** Removing a via lets the drive take the direct road again. */ const removeRoadtripVia = useCallback(async (dayId: number, id: number) => { diff --git a/server/src/nest/roadtrip/roadtrip.controller.ts b/server/src/nest/roadtrip/roadtrip.controller.ts index abe3e1363..6bfd54994 100644 --- a/server/src/nest/roadtrip/roadtrip.controller.ts +++ b/server/src/nest/roadtrip/roadtrip.controller.ts @@ -108,7 +108,7 @@ export class RoadtripController { @Body() body: RoadtripViaUpdateDto, ): { via: RoadtripVia } { this.requireDay(dayId, tripId); - const via = this.roadtrip.move(id, dayId, body.lat, body.lng); + const via = this.roadtrip.move(id, dayId, body.lat, body.lng, body.after_order_index); if (!via) throw new HttpException({ error: 'Via not found' }, 404); return { via }; } diff --git a/server/src/nest/roadtrip/roadtrip.service.ts b/server/src/nest/roadtrip/roadtrip.service.ts index 720f8b853..cd5ae43f1 100644 --- a/server/src/nest/roadtrip/roadtrip.service.ts +++ b/server/src/nest/roadtrip/roadtrip.service.ts @@ -154,14 +154,34 @@ export class RoadtripService { } /** Moving a via is the whole edit; where it sits in the chain does not change. */ - move(id: string | number, dayId: string | number, lat: number, lng: number): RoadtripVia | null { + move( + id: string | number, + dayId: string | number, + lat: number, + lng: number, + afterOrderIndex?: number, + ): RoadtripVia | null { const existing = this.db.get<{ id: number }>( 'SELECT id FROM roadtrip_vias WHERE id = ? AND day_id = ?', id, dayId, ); if (!existing) return null; - this.db.run('UPDATE roadtrip_vias SET lat = ?, lng = ? WHERE id = ?', lat, lng, id); + // The anchor moves with the point when the caller worked out a new one. It is not a + // property of the via but of where the via sits along the drive, so dragging one past + // a stop changes which leg it belongs to — and leaving it behind is what made the + // route run out to the point and back instead of bending through it. + if (afterOrderIndex === undefined) { + this.db.run('UPDATE roadtrip_vias SET lat = ?, lng = ? WHERE id = ?', lat, lng, id); + } else { + this.db.run( + 'UPDATE roadtrip_vias SET lat = ?, lng = ?, after_order_index = ? WHERE id = ?', + lat, + lng, + afterOrderIndex, + id, + ); + } return this.byId(Number(id)); } diff --git a/server/tests/unit/nest/roadtrip.controller.test.ts b/server/tests/unit/nest/roadtrip.controller.test.ts index aaed155f5..22b3cf385 100644 --- a/server/tests/unit/nest/roadtrip.controller.test.ts +++ b/server/tests/unit/nest/roadtrip.controller.test.ts @@ -160,7 +160,15 @@ describe('RoadtripController — writes', () => { it('ROADTRIP-CTL-012: moving a via reports where it went', () => { const s = svc(); expect(new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11 })).toEqual({ via: VIA }); - expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11); + // Undefined anchor, explicitly: a drag that stayed between the same two stops sends + // no new one, and the service must leave the existing pin alone rather than clear it. + expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, undefined); + }); + + it('ROADTRIP-CTL-015: a new anchor is passed on, so a via dragged past a stop is re-pinned', () => { + const s = svc(); + new RoadtripController(s).update('7', '4', '5', { lat: 54, lng: 11, after_order_index: 2 }); + expect(s.move).toHaveBeenCalledWith('5', '4', 54, 11, 2); }); it('ROADTRIP-CTL-013: a via that is not on this day is a 404, not a silent no-op', () => { diff --git a/server/tests/unit/nest/roadtrip.service.test.ts b/server/tests/unit/nest/roadtrip.service.test.ts index dfa73c02c..e047f7c93 100644 --- a/server/tests/unit/nest/roadtrip.service.test.ts +++ b/server/tests/unit/nest/roadtrip.service.test.ts @@ -82,7 +82,10 @@ describe('RoadtripService', () => { expect(service.listForTrip(7).map(v => v.day_id)).toEqual([1, 2]); }); - it('ROADTRIP-SVC-005: moving a via changes where it is, not where it sits in the chain', () => { + it('ROADTRIP-SVC-005: a move with no new anchor leaves the chain alone', () => { + // The drag stayed between the same two stops, so there is nothing to re-pin. The + // sequence is never touched by a move either way: it orders the vias that share one + // anchor, and dragging one does not reorder its neighbours. const via = service.create(1, { after_order_index: 1, lat: 53, lng: 10 }); const moved = service.move(via.id, 1, 52.5, 11.5); @@ -90,6 +93,26 @@ describe('RoadtripService', () => { expect(moved).toMatchObject({ lat: 52.5, lng: 11.5, after_order_index: 1, sequence: via.sequence }); }); + it('ROADTRIP-SVC-007: a via dragged past a stop is re-pinned to the leg it landed on', () => { + // The bug this exists for: a drag used to carry only the coordinates, so a via pulled + // beyond the stop it used to precede kept claiming the earlier leg. The route then ran + // out to the point and back before carrying on, which reads as the drag doing nothing. + const via = service.create(1, { after_order_index: 0, lat: 53.87, lng: 10.7 }); + + const moved = service.move(via.id, 1, 53.87, 11.53, 1); + + expect(moved).toMatchObject({ lat: 53.87, lng: 11.53, after_order_index: 1 }); + }); + + it('ROADTRIP-SVC-008: a re-pin through the wrong day changes nothing at all', () => { + // The day check has to guard the anchor as well as the coordinates, or an id from + // another day could be renumbered through a day the caller does happen to reach. + const via = service.create(1, { after_order_index: 0, lat: 53, lng: 10 }); + + expect(service.move(via.id, 2, 51, 12, 3)).toBeNull(); + expect(service.listForDay(1)[0]).toMatchObject({ lat: 53, lng: 10, after_order_index: 0 }); + }); + it('ROADTRIP-SVC-006: a via cannot be moved or removed through the wrong day', () => { const via = service.create(1, { after_order_index: 0, lat: 53, lng: 10 }); diff --git a/shared/src/roadtrip/roadtrip.schema.ts b/shared/src/roadtrip/roadtrip.schema.ts index 1eac31b1c..8efe3b05e 100644 --- a/shared/src/roadtrip/roadtrip.schema.ts +++ b/shared/src/roadtrip/roadtrip.schema.ts @@ -43,10 +43,20 @@ export const roadtripViaCreateRequestSchema = z.object({ }); export type RoadtripViaCreateRequest = z.infer; -/** Moving a via is the whole edit — dragging it somewhere else on the map. */ +/** + * Moving a via is the whole edit — dragging it somewhere else on the map. + * + * `after_order_index` travels with the drag because the anchor is not a property of the + * point, it is a property of where the point sits along the drive. Without it a via + * dragged past the stop it used to precede keeps claiming the earlier leg, and the route + * runs out to it and back again before carrying on — which reads as the drag having done + * nothing at all. Optional so an older client, and a drag that did not move the via + * across a stop, still send just the coordinates. + */ export const roadtripViaUpdateRequestSchema = z.object({ lat: latSchema, lng: lngSchema, + after_order_index: z.number().int().min(0).optional(), }); export type RoadtripViaUpdateRequest = z.infer;