diff --git a/client/src/store/slices/remoteEventHandler.ts b/client/src/store/slices/remoteEventHandler.ts index 9cdc0979a..1615ce179 100644 --- a/client/src/store/slices/remoteEventHandler.ts +++ b/client/src/store/slices/remoteEventHandler.ts @@ -65,6 +65,9 @@ const putCanonicalBudgetItem: DexieWriter = async (payload, state) => { if (item) await offlineDb.budgetItems.put(item) } const putReservation: DexieWriter = async payload => { + // The same empty ping the appliers above handle; without this the write + // throws and only the swallowed catch keeps it quiet. + if (!payload.reservation) return await offlineDb.reservations.put(payload.reservation as Reservation) } const putTripFile: DexieWriter = async payload => { @@ -451,14 +454,31 @@ export const STATE_APPLIERS: Partial> return {} }, - // Reservations + /* + * ── Reservations ──────────────────────────────────────────────────────── + * + * These two events come in two shapes. Normally they carry the reservation. + * The accommodation cascade sends them empty, as a "something changed, go + * and look" ping, which the event contract allows for. + * + * Reading the id off the empty one is how a trip broke permanently (#1979): + * with no reservations loaded yet, `.some()` never ran the callback, so the + * cast slipped through and the applier returned `[undefined]`. From then on + * every render that walked the list threw "Cannot read properties of + * undefined (reading 'endpoints')" straight into the route error boundary, + * and the trip stayed unreachable in that browser. + */ 'reservation:created': (payload, state) => { - if (state.reservations.some(r => r.id === (payload.reservation as Reservation).id)) return {} - return { reservations: [payload.reservation as Reservation, ...state.reservations] } + const reservation = payload.reservation as Reservation | undefined + if (!reservation) return {} + if (state.reservations.some(r => r.id === reservation.id)) return {} + return { reservations: [reservation, ...state.reservations] } + }, + 'reservation:updated': (payload, state) => { + const reservation = payload.reservation as Reservation | undefined + if (!reservation) return {} + return { reservations: state.reservations.map(r => r.id === reservation.id ? reservation : r) } }, - 'reservation:updated': (payload, state) => ({ - reservations: state.reservations.map(r => r.id === (payload.reservation as Reservation).id ? payload.reservation as Reservation : r), - }), 'reservation:travelers-updated': (payload, state) => ({ reservations: state.reservations.map(r => r.id === (payload.reservationId as number) ? { ...r, travelers: payload.travelers as Reservation['travelers'] } : r), @@ -519,6 +539,18 @@ export function handleRemoteEvent(set: SetState, get: GetState, event: WebSocket } } + /* + * The accommodation cascade announces its reservation without sending it, + * which the applier above correctly declines to invent an entity from. Left + * at that, a hotel booked by one member would not appear for the others + * until they reloaded, so honour the ping the way day:reordered does and + * fetch the authoritative list. + */ + if ((type === 'reservation:created' || type === 'reservation:updated') && !payload.reservation) { + const tripId = get().trip?.id + if (tripId) get().loadReservations(tripId) + } + // A reorder/insert re-pins dates and re-stamps booking times server-side, so // pull the authoritative days + reservations for collaborators. if (type === 'day:reordered') { diff --git a/client/src/utils/reservationRoutes.ts b/client/src/utils/reservationRoutes.ts index b0493e503..312e0c12f 100644 --- a/client/src/utils/reservationRoutes.ts +++ b/client/src/utils/reservationRoutes.ts @@ -1,8 +1,15 @@ import type { Reservation } from '../types' -/** A reservation is routable on the map once it has at least two ordered endpoints (from/to/stop). */ -export function isRoutableReservation(r: Pick): boolean { - return (r.endpoints || []).length >= 2 +/** + * A reservation is routable on the map once it has at least two ordered + * endpoints (from/to/stop). + * + * Total in its argument, because it is handed straight to Array.filter over a + * list this module does not own. It used to guard the property but not the + * element, so one undefined entry took the whole trip planner down (#1979). + */ +export function isRoutableReservation(r: Pick | null | undefined): boolean { + return (r?.endpoints || []).length >= 2 } export interface RouteVisibilityOptions { diff --git a/client/tests/unit/remoteEventHandler/reservationPing.test.ts b/client/tests/unit/remoteEventHandler/reservationPing.test.ts new file mode 100644 index 000000000..2f4e6dece --- /dev/null +++ b/client/tests/unit/remoteEventHandler/reservationPing.test.ts @@ -0,0 +1,94 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { useTripStore } from '../../../src/store/tripStore'; +import { resetAllStores } from '../../helpers/store'; +import { buildReservation, buildTrip } from '../../helpers/factories'; +import { isRoutableReservation } from '../../../src/utils/reservationRoutes'; + +/** + * The reservation event that carries no reservation (#1979). + * + * `reservation:created` and `reservation:updated` come in two shapes: normally + * they carry the row, but the accommodation cascade sends them empty as a + * "something changed, go and look" ping, which the event contract allows. + * + * Reading the id off the empty one is how a trip broke for good. With no + * reservations loaded yet, `.some()` never ran its callback, so the cast slid + * through and the store became `[undefined]`. Every later render that walked + * that list threw into the route error boundary, and because the store is + * rebuilt from the same events on the next visit, the trip stayed unreachable. + * The reporter could only escape it by clearing localStorage. + * + * So the cases below are about the empty shape specifically: it must change + * nothing, it must not leave a hole in the list, and it must still cause the + * new booking to appear. + */ + +beforeEach(() => { + resetAllStores(); +}); + +const send = (type: string, payload: Record = {}) => + useTripStore.getState().handleRemoteEvent({ type, ...payload } as never); + +describe('a reservation event with no reservation on it', () => { + it('leaves an empty list empty, rather than putting a hole in it', () => { + useTripStore.setState({ reservations: [] }); + send('reservation:created'); + // Before the fix this was [undefined], and every render after it threw. + expect(useTripStore.getState().reservations).toEqual([]); + }); + + it('leaves a loaded list untouched', () => { + const existing = buildReservation({ id: 5 }); + useTripStore.setState({ reservations: [existing] }); + send('reservation:updated'); + expect(useTripStore.getState().reservations).toEqual([existing]); + }); + + it('still fetches the list, so a cascaded booking appears without a reload', () => { + const loadReservations = vi.fn(); + useTripStore.setState({ + trip: buildTrip({ id: 42 }), + reservations: [], + loadReservations, + } as never); + + send('reservation:created'); + expect(loadReservations).toHaveBeenCalledWith(42); + }); + + it('does not fetch when the event carries its reservation', () => { + const loadReservations = vi.fn(); + useTripStore.setState({ + trip: buildTrip({ id: 42 }), + reservations: [], + loadReservations, + } as never); + + send('reservation:created', { reservation: buildReservation({ id: 9 }) }); + expect(loadReservations).not.toHaveBeenCalled(); + expect(useTripStore.getState().reservations).toHaveLength(1); + }); +}); + +/* + * The second half of the same crash: the predicate that walks the list is + * handed straight to Array.filter, so it has to survive whatever is in there. + * It guarded the property but not the element. + */ +describe('the routable-reservation predicate', () => { + it('says no to nothing at all, rather than throwing', () => { + expect(isRoutableReservation(undefined)).toBe(false); + expect(isRoutableReservation(null)).toBe(false); + }); + + it('survives a list with a hole in it, which is the reported stack', () => { + const list = [buildReservation({ id: 1, endpoints: [] }), undefined, buildReservation({ id: 2 })]; + expect(() => list.filter(isRoutableReservation)).not.toThrow(); + }); + + it('still answers the ordinary questions the same way', () => { + expect(isRoutableReservation(buildReservation({ id: 1, endpoints: [] }))).toBe(false); + expect(isRoutableReservation({ endpoints: [{ id: 1 }, { id: 2 }] } as never)).toBe(true); + }); +});