From 12f08e36461ada4740b46037f6516c06fab045fa Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 12 Oct 2026 12:33:12 +0200 Subject: [PATCH] fix(planner): a timed booking dropped against its time goes back alone instead of taking the stops behind the drop along The day list draws a row with a time by that time, and an untimed stop takes the time of the timed row before it. A transit ride dropped behind a later stop was stored there, drawn back at its time on the next render, and every untimed stop behind the drop went with it; the next drag then stored that shuffle. A dropped booking or timed note now lands in the slot nearest the drop the list draws it in, so nothing else moves and what is stored is what is shown. Rides dropped where their time allows stay on the drop, and the rule for untimed stops behind a booking is unchanged. The chronology check for a timed stop reads a booking's time for the day (the arrival on the last day of a multi-day ride) instead of its departure, and the three copies of the remove and insert step share one helper. Refs #2631 --- client/scripts/size-baseline.json | 2 +- .../Planner/DayPlanSidebar.dragData.test.tsx | 75 ++++++++++++- .../src/components/Planner/DayPlanSidebar.tsx | 40 ++----- client/src/utils/dayMerge.test.ts | 105 +++++++++++++++++- client/src/utils/dayMerge.ts | 37 ++++++ client/src/utils/mergedOrder.test.ts | 14 ++- client/src/utils/mergedOrder.ts | 19 ++++ 7 files changed, 253 insertions(+), 39 deletions(-) diff --git a/client/scripts/size-baseline.json b/client/scripts/size-baseline.json index 73a490e91..7b2d75aec 100644 --- a/client/scripts/size-baseline.json +++ b/client/scripts/size-baseline.json @@ -12,7 +12,7 @@ "src/components/PDF/TripPDF.tsx": 1097, "src/components/Planner/DayDetailPanel.test.tsx": 2333, "src/components/Planner/DayPlanSidebar.test.tsx": 5381, - "src/components/Planner/DayPlanSidebar.tsx": 3000, + "src/components/Planner/DayPlanSidebar.tsx": 2979, "src/components/Planner/PlaceFormModal.test.tsx": 2864, "src/components/Planner/PlaceFormModal.tsx": 1250, "src/components/Planner/PlaceInspector.tsx": 1308, diff --git a/client/src/components/Planner/DayPlanSidebar.dragData.test.tsx b/client/src/components/Planner/DayPlanSidebar.dragData.test.tsx index 30a297e08..45f878047 100644 --- a/client/src/components/Planner/DayPlanSidebar.dragData.test.tsx +++ b/client/src/components/Planner/DayPlanSidebar.dragData.test.tsx @@ -1,10 +1,19 @@ -// FE-PLANNER-DAYPLAN-236: the drag data a booking drop leaves behind. The mocks -// are the ones DayPlanSidebar.test.tsx renders the sidebar with. +// FE-PLANNER-DAYPLAN-236: the drag data a booking drop leaves behind, and +// FE-PLANNER-DAYPLAN-237/238: where a timed booking dropped in the day lands. The +// mocks are the ones DayPlanSidebar.test.tsx renders the sidebar with. import { http, HttpResponse } from 'msw'; -import { buildDay, buildReservation, buildTrip, buildUser } from '../../../tests/helpers/factories'; +import { + buildAssignment, + buildDay, + buildPlace, + buildReservation, + buildTrip, + buildUser, +} from '../../../tests/helpers/factories'; import { server } from '../../../tests/helpers/msw/server'; -import { fireEvent, render, screen } from '../../../tests/helpers/render'; +import { fireEvent, render, screen, waitFor } from '../../../tests/helpers/render'; import { resetAllStores, seedStore } from '../../../tests/helpers/store'; +import { reservationsApi } from '../../api/client'; import { clearExchangeRateCache } from '../../hooks/useExchangeRates'; import { useAuthStore } from '../../store/authStore'; import { useSettingsStore } from '../../store/settingsStore'; @@ -183,3 +192,61 @@ describe('DayPlanSidebar drag data', () => { expect(window.__dragData).toBeNull(); }); }); + +// #2631: a booking with a time is drawn by it, and the untimed stops behind it take +// that time. Dropped against its time it went back on the next render and took the +// stops behind the drop with it, so the day reshuffled on every drag. +describe('DayPlanSidebar timed booking drop', () => { + const renderDay = (busTime: string, busSlot: number) => { + const day = buildDay({ id: 10, date: '2025-06-01', title: 'Day 1' }); + const stops = [ + buildPlace({ id: 1, name: 'Breakfast', place_time: '08:00' }), + buildPlace({ id: 2, name: 'Museum', place_time: '14:00' }), + buildPlace({ id: 3, name: 'Old town' }), + buildPlace({ id: 4, name: 'Harbour' }), + ]; + const assignments = { + '10': stops.map((place, i) => buildAssignment({ id: 11 + i, day_id: 10, order_index: i, place })), + }; + const bus = buildReservation({ + id: 500, + type: 'bus', + title: 'City bus', + day_id: 10, + reservation_time: `2025-06-01T${busTime}`, + day_plan_position: busSlot, + day_positions: { 10: busSlot }, + }); + seedStore(useTripStore, { reservations: [bus] }); + const onReorder = vi.fn(async () => undefined); + render( + + ); + const row = (text: string) => screen.getByText(text).closest('[draggable="true"]') as HTMLElement; + fireEvent.dragStart(row('City bus'), { + dataTransfer: { setData: vi.fn(), effectAllowed: '', getData: vi.fn(() => '') }, + }); + fireEvent.drop(row('Harbour'), { dataTransfer: { getData: vi.fn(() => '') } }); + return { onReorder }; + }; + + it('FE-PLANNER-DAYPLAN-237: a booking dropped against its time stays at its time and moves no stop', async () => { + // The 12:00 bus dropped ahead of Harbour, behind the 14:00 museum. + const { onReorder } = renderDay('12:00', 0.5); + await waitFor(() => + expect(reservationsApi.updatePositions).toHaveBeenCalledWith(1, [{ id: 500, day_plan_position: 0.5 }], 10) + ); + expect(onReorder).toHaveBeenCalledWith(10, [11, 12, 13, 14]); + expect(useTripStore.getState().reservations[0].day_positions).toEqual({ 10: 0.5 }); + }); + + it('FE-PLANNER-DAYPLAN-238: a booking dropped where its time allows lands on the drop', async () => { + const { onReorder } = renderDay('16:00', 1.5); + await waitFor(() => + expect(reservationsApi.updatePositions).toHaveBeenCalledWith(1, [{ id: 500, day_plan_position: 2.5 }], 10) + ); + expect(onReorder).toHaveBeenCalledWith(10, [11, 12, 13, 14]); + }); +}); diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index 7cfc50e89..b78a57284 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -46,7 +46,7 @@ import { getAccommodationAnchors, getDayBookendHotels, shouldDrawMorningLeg, sho import { TRANSPORT_TYPES, parseTimeToMinutes, getSpanPhase, hidesOnMiddleDay, getDisplayTimeForDay, getTransportRouteEndpoints, getTransportForDay as _getTransportForDay, getMergedItems as _getMergedItems, isCarrierTransport, hasCarrierEndpointOnDay, - getAssignmentReservations, timedSlot, storedRideSlot, + getAssignmentReservations, timedSlot, storedRideSlot, seatTimedDrop, type MergedItem, } from '../../utils/dayMerge' import { withinDriveRange } from '@trek/shared/roadtrip' @@ -54,7 +54,7 @@ import { formatDate, formatTime, formatMoneySum, splitReservationDateTime } from import { dayHeadingParts } from '../../utils/dayLabel' import { pendingStayIds } from '../../utils/pendingStays' import { dayWeatherAnchor } from '../../utils/dayWeather' -import { metadataWithLegPositions, planMergedOrder, withTransportPositions } from '../../utils/mergedOrder' +import { metadataWithLegPositions, moveRow, planMergedOrder, withTransportPositions } from '../../utils/mergedOrder' import { planCosts } from './planCosts' import { useDayNotes } from '../../hooks/useDayNotes' import { useExchangeRates } from '../../hooks/useExchangeRates' @@ -939,16 +939,9 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const fromIdx = m.findIndex(i => i.type === fromType && i.data.id === fromId) const toIdx = m.findIndex(matchTo) if (fromIdx !== -1 && toIdx !== -1) { - const simulated = [...m] - const [moved] = simulated.splice(fromIdx, 1) - let insertIdx = simulated.findIndex(matchTo) - if (insertIdx === -1) insertIdx = simulated.length - if (insertAfter) insertIdx += 1 - simulated.splice(insertIdx, 0, moved) - - const timedInOrder = simulated + const timedInOrder = moveRow(m, fromIdx, matchTo, insertAfter).order .map(i => { - if (i.type === 'transport') return parseTimeToMinutes(i.data?.reservation_time) + if (i.type === 'transport') return parseTimeToMinutes(getDisplayTimeForDay(i.data, dayId)) if (i.type === 'place') return parseTimeToMinutes(i.data?.place?.place_time) return null }) @@ -974,17 +967,9 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { return } - const newOrder = [...m] - const [moved] = newOrder.splice(fromIdx, 1) - let adjustedTo = newOrder.findIndex(matchTo) - if (adjustedTo === -1) adjustedTo = newOrder.length - if (insertAfter) adjustedTo += 1 - newOrder.splice(adjustedTo, 0, moved) - - await applyMergedOrder(dayId, newOrder) - setDraggingId(null) - setDropTargetKey(null) - dragDataRef.current = null + const dropped = moveRow(m, fromIdx, matchTo, insertAfter) + await applyMergedOrder(dayId, seatTimedDrop(dropped.order, dropped.index, dayId, getDisplayTimeForDay)) // #2631 + setDraggingId(null); setDropTargetKey(null); dragDataRef.current = null } const confirmTimeRemoval = async () => { @@ -1011,14 +996,7 @@ function useDayPlanSidebar(props: DayPlanSidebarProps) { const toIdx = m.findIndex(matchTo) if (fromIdx === -1 || toIdx === -1 || fromIdx === toIdx) return - const newOrder = [...m] - const [moved] = newOrder.splice(fromIdx, 1) - let adjustedTo = newOrder.findIndex(matchTo) - if (adjustedTo === -1) adjustedTo = newOrder.length - if (insertAfter) adjustedTo += 1 - newOrder.splice(adjustedTo, 0, moved) - - await applyMergedOrder(dayId, newOrder) + await applyMergedOrder(dayId, moveRow(m, fromIdx, matchTo, insertAfter).order) } } @@ -1866,7 +1844,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP if (parseTimeToMinutes(placeTime) !== null) { const timedInNewOrder = newOrder .map(i => { - if (i.type === 'transport') return parseTimeToMinutes(i.data?.reservation_time) + if (i.type === 'transport') return parseTimeToMinutes(getDisplayTimeForDay(i.data, day.id)) if (i.type === 'place') return parseTimeToMinutes(i.data?.place?.place_time) return null }) diff --git a/client/src/utils/dayMerge.test.ts b/client/src/utils/dayMerge.test.ts index fe107538b..131a10320 100644 --- a/client/src/utils/dayMerge.test.ts +++ b/client/src/utils/dayMerge.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest' -import { parseTimeToMinutes, getSpanPhase, hidesOnMiddleDay, getTransportRouteEndpoints, getDisplayTimeForDay, getTransportForDay, getAssignmentReservations, getPlaceBookings, getMergedItems, rideSeatKey, stayStartingOn, storedRideSlot, timedSlot } from './dayMerge' +import { parseTimeToMinutes, getSpanPhase, hidesOnMiddleDay, getTransportRouteEndpoints, getDisplayTimeForDay, getTransportForDay, getAssignmentReservations, getPlaceBookings, getMergedItems, rideSeatKey, seatTimedDrop, stayStartingOn, storedRideSlot, timedSlot, type MergedItem } from './dayMerge' +import { planMergedOrder, withTransportPositions } from './mergedOrder' describe('parseTimeToMinutes', () => { it('parses HH:MM string', () => { @@ -491,3 +492,105 @@ describe('timedSlot', () => { expect(timedSlot(leading, nights, '08:00')).toBe(1) }) }) + +// #2631: a day with timed and untimed stops between timed transit rides. Dropping a +// ride against its time used to draw it back at its time with every untimed stop +// behind the drop, and the next drag stored that shuffle. +describe('seatTimedDrop', () => { + const DAY = 1 + const stop = (id: number, name: string, time: string | null = null) => + ({ id, order_index: 0, place: { name, place_time: time } }) + const ride = (id: number, time: string, pos: number) => + ({ id, type: 'transit', title: `ride ${time}`, day_id: DAY, reservation_time: `2026-11-20T${time}`, day_positions: { [DAY]: pos } }) + const note = (id: number, time: string | null, sort_order: number) => ({ id, text: `note ${id}`, time, sort_order }) + + const setup = () => { + const stops = [ + stop(1, 'Palacio', '07:30'), stop(2, 'Jardin', '10:30'), stop(3, 'Asamkirche', '14:00'), stop(4, 'StPeter', '14:30'), + stop(5, 'Sendlinger'), stop(6, 'Ayuntamiento'), stop(7, 'Isartor'), stop(8, 'Juliet'), + ].map((a, i) => ({ ...a, order_index: i })) + return { stops, rides: [ride(101, '07:01', -0.5), ride(102, '12:01', 1.5), ride(103, '16:55', 3.5)], notes: [] as ReturnType[] } + } + type Day = ReturnType + const draw = (day: Day) => getMergedItems({ dayAssignments: day.stops, dayNotes: day.notes, dayTransports: day.rides, dayId: DAY }) + const label = (it: MergedItem) => it.type === 'place' ? it.data.place.name : it.type === 'note' ? it.data.text : it.data.title + + /** What the day list does with a drop: insert, seat, store, and draw again. */ + const drop = (day: Day, from: (it: MergedItem) => boolean, to: (it: MergedItem) => boolean, after = true) => { + const m = draw(day) + const order = [...m] + const [moved] = order.splice(order.findIndex(from), 1) + const at = order.findIndex(to) + (after ? 1 : 0) + order.splice(at, 0, moved) + const seated = seatTimedDrop(order, at, DAY) + const plan = planMergedOrder(seated) + const stored: Day = { + stops: day.stops.map(a => ({ ...a, order_index: plan.assignmentIds.indexOf(a.id) })), + rides: withTransportPositions(day.rides, plan.transportUpdates, DAY), + notes: day.notes.map(n => ({ ...n, sort_order: plan.noteUpdates.find(u => u.id === n.id)?.sort_order ?? n.sort_order })), + } + return { seated: seated.map(label), drawn: draw(stored).map(label), stored } + } + const isRide = (id: number) => (it: MergedItem) => it.type === 'transport' && it.data.id === id + const isStop = (name: string) => (it: MergedItem) => it.type === 'place' && it.data.place.name === name + const stopsOf = (labels: string[]) => labels.filter(l => !l.startsWith('ride') && !l.startsWith('note')) + + it('draws the day as the setup expects', () => { + expect(draw(setup()).map(label)).toEqual([ + 'ride 07:01', 'Palacio', 'Jardin', 'ride 12:01', 'Asamkirche', 'StPeter', 'ride 16:55', 'Sendlinger', 'Ayuntamiento', 'Isartor', 'Juliet', + ]) + }) + + it('a ride dropped against its time goes back alone and the stops keep their order', () => { + const day = setup() + const before = stopsOf(draw(day).map(label)) + const { seated, drawn } = drop(day, isRide(102), isStop('Sendlinger')) + expect(drawn).toEqual(seated) + expect(stopsOf(drawn)).toEqual(before) + expect(drawn.indexOf('ride 12:01')).toBe(drawn.indexOf('Asamkirche') - 1) + }) + + it('a later drag of another ride does not reshuffle the stops either', () => { + const first = drop(setup(), isRide(102), isStop('Sendlinger')) + const { seated, drawn } = drop(first.stored, isRide(101), isStop('Ayuntamiento')) + expect(drawn).toEqual(seated) + expect(stopsOf(drawn)).toEqual(['Palacio', 'Jardin', 'Asamkirche', 'StPeter', 'Sendlinger', 'Ayuntamiento', 'Isartor', 'Juliet']) + expect(drawn[0]).toBe('ride 07:01') + }) + + it('a ride dropped between untimed stops its time allows stays where it was dropped', () => { + const { seated, drawn } = drop(setup(), isRide(103), isStop('Isartor')) + expect(drawn).toEqual(seated) + expect(drawn.slice(-4)).toEqual(['Ayuntamiento', 'Isartor', 'ride 16:55', 'Juliet']) + }) + + it('leaves the untimed stops behind a ride where they are, as before', () => { + const day: Day = { + stops: [stop(1, 'Hotel'), stop(2, 'Museum'), stop(3, 'Dinner')].map((a, i) => ({ ...a, order_index: i })), + rides: [ride(101, '09:00', 0.5)], + notes: [], + } + expect(draw(day).map(label)).toEqual(['Hotel', 'ride 09:00', 'Museum', 'Dinner']) + const { seated, drawn } = drop(day, isRide(101), isStop('Museum')) + expect(drawn).toEqual(seated) + expect(drawn).toEqual(['Hotel', 'Museum', 'ride 09:00', 'Dinner']) + }) + + it('seats a timed note the same way and leaves an untimed one where it was dropped', () => { + const day = setup() + day.notes = [note(201, '12:30', 1.6), note(202, null, 6.5)] + const isNote = (id: number) => (it: MergedItem) => it.type === 'note' && it.data.id === id + const timed = drop(day, isNote(201), isStop('Isartor')) + expect(timed.drawn).toEqual(timed.seated) + expect(timed.drawn.indexOf('note 201')).toBe(timed.drawn.indexOf('Asamkirche') - 1) + const untimed = drop(day, isNote(202), isStop('Palacio')) + expect(untimed.drawn).toEqual(untimed.seated) + expect(untimed.drawn.indexOf('note 202')).toBe(untimed.drawn.indexOf('Palacio') + 1) + }) + + it('returns a dropped stop as it came', () => { + const order = draw(setup()) + expect(seatTimedDrop(order, 1, DAY)).toEqual(order) + }) +}) + diff --git a/client/src/utils/dayMerge.ts b/client/src/utils/dayMerge.ts index 1d015ecf4..bb0462fdf 100644 --- a/client/src/utils/dayMerge.ts +++ b/client/src/utils/dayMerge.ts @@ -309,6 +309,43 @@ function applyChronoOrder( }) } +/** + * A day order with a dragged booking or note at `index`, moved to the nearest slot the + * list draws it in. A row with a time is drawn by it, and the untimed rows behind it + * take that time too. Dropped against its time, a train went back to its time on the + * next render and took every untimed stop behind the drop with it, and the next drag + * stored that order (#2631). So it lands as close to the drop as its time allows and + * nothing else moves. A row without a time, or one the list draws where it was + * dropped, is returned as it came. + */ +export function seatTimedDrop( + order: readonly MergedItem[], + index: number, + dayId: number, + getDisplayTime: (r: MergedItem['data'], dayId: number) => string | null = getDisplayTimeForDay +): MergedItem[] { + const moved = order[index] + if (!moved || moved.type === 'place') return [...order] + const minutes = moved.type === 'note' + ? parseTimeToMinutes(moved.data?.time) + : parseTimeToMinutes(getDisplayTime(moved.data, dayId)) + if (minutes === null) return [...order] + const rest = order.filter((_, i) => i !== index) + const drawnAsIs = (at: number): MergedItem[] | null => { + const candidate = [...rest.slice(0, at), moved, ...rest.slice(at)] + const drawn = applyChronoOrder(candidate, dayId, getDisplayTime) + return drawn.every((it, i) => it === candidate[i]) ? candidate : null + } + for (let dist = 0; dist <= rest.length; dist++) { + for (const at of dist === 0 ? [index] : [index - dist, index + dist]) { + if (at < 0 || at > rest.length) continue + const seated = drawnAsIs(at) + if (seated) return seated + } + } + return [...order] +} + /** A row of the day list as the seat of a ride reads it; a note is none (see rideSeatKey). */ function seatItemsOf(row: MergedItem): SeatItem[] { if (row.type === 'place') { diff --git a/client/src/utils/mergedOrder.test.ts b/client/src/utils/mergedOrder.test.ts index 62f83cf8d..b7e7ff652 100644 --- a/client/src/utils/mergedOrder.test.ts +++ b/client/src/utils/mergedOrder.test.ts @@ -1,8 +1,8 @@ -// FE-UTIL-MERGEDORDER-001 to -007: the reorder writes the desktop day plan and the +// FE-UTIL-MERGEDORDER-001 to -008: the reorder writes the desktop day plan and the // phone's plan timeline both derive from a day's new timeline order. import { describe, expect, it } from 'vitest'; -import { metadataWithLegPositions, planMergedOrder, withTransportPositions } from './mergedOrder'; +import { metadataWithLegPositions, moveRow, planMergedOrder, withTransportPositions } from './mergedOrder'; const place = (id: number) => ({ type: 'place', data: { id } }); const note = (id: number) => ({ type: 'note', data: { id } }); @@ -81,3 +81,13 @@ describe('metadataWithLegPositions', () => { expect(metadataWithLegPositions('', { 0: 1 }, 4)).toBeNull(); }); }); + +describe('moveRow', () => { + it('FE-UTIL-MERGEDORDER-008: puts the row in front of the target, behind it with after, at the end without one', () => { + const rows = ['a', 'b', 'c', 'd']; + expect(moveRow(rows, 3, (r) => r === 'b', false)).toEqual({ order: ['a', 'd', 'b', 'c'], index: 1 }); + expect(moveRow(rows, 0, (r) => r === 'c', true)).toEqual({ order: ['b', 'c', 'a', 'd'], index: 2 }); + expect(moveRow(rows, 1, () => false, false)).toEqual({ order: ['a', 'c', 'd', 'b'], index: 3 }); + expect(rows).toEqual(['a', 'b', 'c', 'd']); + }); +}); diff --git a/client/src/utils/mergedOrder.ts b/client/src/utils/mergedOrder.ts index 7562e084e..2724566b0 100644 --- a/client/src/utils/mergedOrder.ts +++ b/client/src/utils/mergedOrder.ts @@ -23,6 +23,25 @@ export interface MergedOrderPlan { legPosUpdates: Record>; } +/** + * The day order with the row at `from` taken out and put back in front of the first + * row `isTarget` picks, or behind it with `after`; at the end when no row is picked. + * `index` is where the row landed. + */ +export function moveRow( + order: readonly T[], + from: number, + isTarget: (row: T) => boolean, + after = false +): { order: T[]; index: number } { + const moved = [...order]; + const [row] = moved.splice(from, 1); + const target = moved.findIndex(isTarget); + const index = (target === -1 ? moved.length : target) + (after ? 1 : 0); + moved.splice(index, 0, row); + return { order: moved, index }; +} + export function planMergedOrder(newOrder: readonly MergedOrderItem[]): MergedOrderPlan { const plan: MergedOrderPlan = { assignmentIds: [], noteUpdates: [], transportUpdates: [], legPosUpdates: {} }; let placeCount = 0;