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;