From 5daa2a0311795982c7287ff542becdcdad023147 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 22 Oct 2026 00:03:28 +0000 Subject: [PATCH] fix(planner): a stay booked elsewhere shows without a reload The server sends accommodation:created and accommodation:updated to the whole trip, the assistant's writes included, but the client listed both as ignored and reloaded stays only after a delete. A stay the assistant, a fellow traveller, a plugin or a booking import added therefore showed in the sidebar, on the map and in the day panel only after a reload. Both events now refresh the stays like the deletes do, in the planner (desktop and phone, which share its hook) and in the day panel. The refresh is bundled per burst, since one assistant step can apply many changes, a newer answer always wins over an older one, and a stay being edited in the day panel that was deleted or moved elsewhere closes its editor instead of letting the form overwrite another stay. A browser test books a stay from a second context and sees it appear in the open planner, on the desktop and on the phone. --- client/e2e/stay-live.spec.ts | 93 +++++++++++++ client/scripts/eslint-baseline.json | 8 +- client/src/api/wsEventPolicy.ts | 14 +- .../components/Planner/useDayDetail.test.ts | 125 ++++++++++++++++- client/src/components/Planner/useDayDetail.ts | 54 ++++++-- .../Planner/useStaysRefresh.test.ts | 127 ++++++++++++++++++ .../src/components/Planner/useStaysRefresh.ts | 76 +++++++++++ .../components/Planner/useTripStays.test.ts | 92 +++++++++++++ client/src/components/Planner/useTripStays.ts | 46 +++++++ .../pages/tripPlanner/useTripPlanner.test.tsx | 16 +-- .../src/pages/tripPlanner/useTripPlanner.ts | 24 +--- client/src/store/slices/remoteEventHandler.ts | 30 +++-- .../remoteEventHandler/accommodations.test.ts | 52 +++++++ 13 files changed, 703 insertions(+), 54 deletions(-) create mode 100644 client/e2e/stay-live.spec.ts create mode 100644 client/src/components/Planner/useStaysRefresh.test.ts create mode 100644 client/src/components/Planner/useStaysRefresh.ts create mode 100644 client/src/components/Planner/useTripStays.test.ts create mode 100644 client/src/components/Planner/useTripStays.ts create mode 100644 client/tests/unit/remoteEventHandler/accommodations.test.ts diff --git a/client/e2e/stay-live.spec.ts b/client/e2e/stay-live.spec.ts new file mode 100644 index 000000000..cca59f3e0 --- /dev/null +++ b/client/e2e/stay-live.spec.ts @@ -0,0 +1,93 @@ +import type { APIRequestContext, Browser, Page } from '@playwright/test'; +import { devices, expect, test } from '@playwright/test'; +import { E2E_STATE_FILE } from '../playwright.config'; + +// A stay booked somewhere else shows in the open planner without a reload. The +// assistant books through its tools, and a stay it booked showed in the planner +// only after a reload: the server told every tab, the client dropped the news. +// Here a second session (a browser context of its own, so the open planner's +// socket is not the one that asked) books the stay over the API, and the planner +// has to show it on its own, on the desktop and on the phone. + +/** A trip with three dated days, and their ids in order. */ +async function seedTrip(api: APIRequestContext, title: string) { + const created = await api.post('/api/trips', { + data: { title, start_date: '2099-07-01', end_date: '2099-07-03' }, + }); + expect(created.ok(), 'seed trip').toBeTruthy(); + const body = await created.json(); + const tripId: number = body.trip?.id ?? body.id; + const days = await (await api.get(`/api/trips/${tripId}/days`)).json(); + const dayIds: number[] = (days.days ?? days).map((day: { id: number }) => day.id); + expect(dayIds.length, 'the trip has its three days').toBe(3); + return { tripId, dayIds }; +} + +/** Opens the planner and waits until its socket joined the trip's room, so no event can slip past it. */ +async function openPlanner(page: Page, tripId: number) { + const joined = new Promise((resolve) => { + page.on('websocket', (ws) => { + ws.on('framereceived', (frame) => { + if (String(frame.payload).includes('"joined"')) resolve(); + }); + }); + }); + await page.goto(`/trips/${tripId}`); + await joined; +} + +/** Books a hotel from the first to the second day, from a session of its own. */ +async function bookElsewhere(browser: Browser, tripId: number, dayIds: number[], hotel: string) { + const other = await browser.newContext({ storageState: E2E_STATE_FILE }); + try { + const place = await other.request.post(`/api/trips/${tripId}/places`, { + data: { name: hotel, lat: 52.5306, lng: 13.3846 }, + }); + expect(place.ok(), 'the hotel place').toBeTruthy(); + const placeBody = await place.json(); + const stay = await other.request.post(`/api/trips/${tripId}/accommodations`, { + data: { place_id: placeBody.place?.id ?? placeBody.id, start_day_id: dayIds[0], end_day_id: dayIds[1] }, + }); + expect(stay.ok(), 'the stay').toBeTruthy(); + } finally { + await other.close(); + } +} + +test('desktop: a stay booked in another session shows in the day plan without a reload', async ({ page, browser }) => { + const hotel = `Mercure Hotel ${Date.now()}`; + const { tripId, dayIds } = await seedTrip(page.request, `Stay live ${Date.now()}`); + await openPlanner(page, tripId); + await expect(page.locator('.leaflet-container')).toBeVisible({ timeout: 20_000 }); + + await bookElsewhere(browser, tripId, dayIds, hotel); + + // The day head names the hotel of the night, read from the planner's stays. + await expect(page.locator('[data-dp="day-pill"]', { hasText: hotel }).first()).toBeVisible({ timeout: 10_000 }); +}); + +test('phone: a stay booked in another session shows on the day plan without a reload', async ({ browser }) => { + const pixel = devices['Pixel 7']; + const phone = await browser.newContext({ + viewport: pixel.viewport, + userAgent: pixel.userAgent, + deviceScaleFactor: pixel.deviceScaleFactor, + isMobile: pixel.isMobile, + hasTouch: pixel.hasTouch, + storageState: E2E_STATE_FILE, + }); + try { + const page = await phone.newPage(); + const hotel = `Mercure Hotel ${Date.now()}`; + const { tripId, dayIds } = await seedTrip(page.request, `Stay live phone ${Date.now()}`); + await openPlanner(page, tripId); + await expect(page.locator('button[aria-current="true"]')).toBeVisible({ timeout: 30_000 }); + + await bookElsewhere(browser, tripId, dayIds, hotel); + + // The first day's plan carries a chip for the hotel checked into that night. + await expect(page.locator('button', { hasText: hotel }).first()).toBeVisible({ timeout: 10_000 }); + } finally { + await phone.close(); + } +}); diff --git a/client/scripts/eslint-baseline.json b/client/scripts/eslint-baseline.json index 60488f868..d6ecc9098 100644 --- a/client/scripts/eslint-baseline.json +++ b/client/scripts/eslint-baseline.json @@ -5,13 +5,13 @@ "no-console": 88, "complexity": 230, "@typescript-eslint/no-explicit-any": 147, - "trek/no-swallowed-catch": 133, + "trek/no-swallowed-catch": 132, "max-params": 35, "trek/store-without-selector": 69, "react-hooks/refs": 557, "react-hooks/set-state-in-effect": 196, - "@typescript-eslint/no-floating-promises": 167, - "react-hooks/exhaustive-deps": 121, + "@typescript-eslint/no-floating-promises": 166, + "react-hooks/exhaustive-deps": 119, "@typescript-eslint/no-unused-vars": 127, "react-hooks/immutability": 28, "max-lines-per-function": 133, @@ -39,7 +39,7 @@ "@typescript-eslint/no-misused-promises": 22, "@typescript-eslint/no-unused-vars": 51, "@typescript-eslint/no-floating-promises": 33, - "trek/no-window-globals": 332, + "trek/no-window-globals": 331, "trek/no-swallowed-catch": 14, "react-hooks/globals": 5, "react-hooks/refs": 22, diff --git a/client/src/api/wsEventPolicy.ts b/client/src/api/wsEventPolicy.ts index db6bf4465..07bb2c16e 100644 --- a/client/src/api/wsEventPolicy.ts +++ b/client/src/api/wsEventPolicy.ts @@ -106,6 +106,15 @@ export const HANDLED_OUTSIDE_TRIP_STORE = [ 'import:progress', 'import:done', 'import:error', + // Stays live in page-local planner state, not in the trip store. A stay + // written elsewhere (the assistant, a fellow traveller, a plugin, a booking + // import) goes remoteEventHandler (STAY_REFRESH_EVENTS) -> accommodations:refresh + // -> components/Planner/useStaysRefresh, which reloads them once per burst in + // pages/tripPlanner/useTripPlanner (sidebar, map, the phone's day sheet) and + // components/Planner/useDayDetail (the desktop day panel). + 'accommodation:created', + 'accommodation:updated', + 'accommodation:deleted', ] as const satisfies readonly TrekWsEventName[] /** @@ -129,11 +138,6 @@ export const IGNORED_WS_EVENTS = [ 'budget:settlement-created', 'budget:settlement-updated', 'budget:settlement-deleted', - // Accommodations live in page-local planner state; the client refetches - // them off trip:updated date changes, never off these events. - 'accommodation:created', - 'accommodation:updated', - 'accommodation:deleted', 'trip:deleted', 'member:added', 'member:removed', diff --git a/client/src/components/Planner/useDayDetail.test.ts b/client/src/components/Planner/useDayDetail.test.ts index 812116285..8ef9a6349 100644 --- a/client/src/components/Planner/useDayDetail.test.ts +++ b/client/src/components/Planner/useDayDetail.test.ts @@ -1,13 +1,19 @@ // FE-PLANNER-USEDAYDETAIL-001 to -009: the forecast, the in-place rename and the // stay write the desktop day panel and the phone day and stay sheets share. +// FE-PLANNER-USEDAYDETAIL-010 to -014: the desktop day panel's own stays follow +// a stay written elsewhere without a reload, and the stay open in its editor is +// never swapped for another one to save over. import { act, renderHook, waitFor } from '@testing-library/react'; import type { WeatherResult } from '@trek/shared'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { buildDay } from '../../../tests/helpers/factories'; import { resetAllStores } from '../../../tests/helpers/store'; import { accommodationsApi, weatherApi } from '../../api/client'; +import { accommodationRepo } from '../../repo/accommodationRepo'; import { useTripStore } from '../../store/tripStore'; -import { useDayForecast, useDayRename, writeStay } from './useDayDetail'; +import type { Accommodation } from '../../types'; +import { useDayDetail, useDayForecast, useDayRename, writeStay } from './useDayDetail'; const SUNNY = { temp: 21, main: 'Clear', description: 'clear sky' } as WeatherResult; @@ -138,3 +144,120 @@ describe('writeStay', () => { await expect(writeStay(3, 4, body)).rejects.toThrow('overlap'); }); }); + +describe('useDayDetail stays', () => { + const arrival = buildDay({ id: 1 }); + const departure = buildDay({ id: 2 }); + const days = [arrival, departure]; + const stay = (id: number, startDayId: number, endDayId: number): Accommodation => ({ + id, + trip_id: 3, + place_id: 7, + start_day_id: startDayId, + end_day_id: endDayId, + }); + /** A stay written by someone else, as the trip room hands it to the store. */ + const writtenElsewhere = (type: 'accommodation:created' | 'accommodation:updated', accommodation: Accommodation) => + act(() => useTripStore.getState().handleRemoteEvent({ type, accommodation })); + + it('FE-PLANNER-USEDAYDETAIL-010: stays booked elsewhere show in the day panel after one reload', async () => { + const tonight = stay(8, 1, 2); + const later = stay(9, 2, 2); + const list = vi + .spyOn(accommodationRepo, 'list') + .mockResolvedValueOnce({ accommodations: [] }) + .mockResolvedValue({ accommodations: [tonight, later] }); + const { result } = renderHook(() => useDayDetail(arrival, days, 3, null, null, 'en')); + await waitFor(() => expect(list).toHaveBeenCalledTimes(1)); + + writtenElsewhere('accommodation:created', tonight); + writtenElsewhere('accommodation:created', later); + + await waitFor(() => expect(result.current.dayAccommodations).toEqual([tonight])); + expect(result.current.accommodations).toEqual([tonight, later]); + expect(result.current.accommodation).toEqual(tonight); + expect(list).toHaveBeenCalledTimes(2); + }); + + it('FE-PLANNER-USEDAYDETAIL-011: the stay open for editing stays selected through that reload', async () => { + const first = stay(8, 1, 1); + const second = stay(9, 1, 2); + const list = vi.spyOn(accommodationRepo, 'list').mockResolvedValue({ accommodations: [first, second] }); + const { result } = renderHook(() => useDayDetail(arrival, days, 3, null, null, 'en')); + await waitFor(() => expect(result.current.accommodation).toEqual(first)); + act(() => result.current.setAccommodation(second)); + + const moved = { ...second, check_in: '15:00' }; + list.mockResolvedValue({ accommodations: [first, moved] }); + writtenElsewhere('accommodation:updated', moved); + + await waitFor(() => expect(result.current.accommodation).toEqual(moved)); + expect(result.current.dayAccommodations).toEqual([first, moved]); + }); + + /** + * Opens the editor on the day's second stay, lets someone else take it off the + * day, and hands back what the panel shows afterwards. + */ + async function editedStayLeavesTheDay( + afterwards: Accommodation[], + event: 'accommodation:updated' | 'accommodation:deleted' + ) { + const first = stay(8, 1, 1); + const second = stay(9, 1, 2); + const list = vi.spyOn(accommodationRepo, 'list').mockResolvedValue({ accommodations: [first, second] }); + const update = vi.spyOn(accommodationRepo, 'update'); + const view = renderHook(() => useDayDetail(arrival, days, 3, null, null, 'en')); + await waitFor(() => expect(view.result.current.dayAccommodations).toEqual([first, second])); + act(() => { + view.result.current.setAccommodation(second); + view.result.current.setShowHotelPicker('edit'); + }); + + list.mockResolvedValue({ accommodations: afterwards }); + if (event === 'accommodation:deleted') { + act(() => useTripStore.getState().handleRemoteEvent({ type: event, accommodationId: second.id })); + } else { + writtenElsewhere(event, afterwards[1] ?? second); + } + await waitFor(() => expect(view.result.current.dayAccommodations).toEqual([first])); + return { ...view, update }; + } + + it('FE-PLANNER-USEDAYDETAIL-012: the stay open in the editor, cancelled elsewhere, closes the editor and leaves none to save over', async () => { + const { result, update } = await editedStayLeavesTheDay([stay(8, 1, 1)], 'accommodation:deleted'); + + expect(result.current.accommodation).toBeNull(); + expect(result.current.showHotelPicker).toBe(false); + await act(() => result.current.handleUpdateAccommodation()); + expect(update).not.toHaveBeenCalled(); + }); + + it('FE-PLANNER-USEDAYDETAIL-013: the stay open in the editor, moved to another day elsewhere, closes the editor too', async () => { + const { result } = await editedStayLeavesTheDay([stay(8, 1, 1), stay(9, 2, 2)], 'accommodation:updated'); + + expect(result.current.accommodation).toBeNull(); + expect(result.current.showHotelPicker).toBe(false); + }); + + it("FE-PLANNER-USEDAYDETAIL-014: saving the stay open in the editor keeps it picked, not the day's first", async () => { + const first = stay(8, 1, 1); + const second = stay(9, 1, 2); + const saved = { ...second, check_in: '15:00' }; + const list = vi.spyOn(accommodationRepo, 'list').mockResolvedValue({ accommodations: [first, second] }); + const update = vi.spyOn(accommodationRepo, 'update').mockResolvedValue({ accommodation: saved }); + const { result } = renderHook(() => useDayDetail(arrival, days, 3, null, null, 'en')); + await waitFor(() => expect(result.current.dayAccommodations).toEqual([first, second])); + act(() => { + result.current.setAccommodation(second); + result.current.setShowHotelPicker('edit'); + }); + + list.mockResolvedValue({ accommodations: [first, saved] }); + await act(() => result.current.handleUpdateAccommodation()); + + expect(update).toHaveBeenCalledWith(3, second.id, expect.any(Object)); + expect(result.current.accommodation).toEqual(saved); + expect(result.current.showHotelPicker).toBe(false); + }); +}); diff --git a/client/src/components/Planner/useDayDetail.ts b/client/src/components/Planner/useDayDetail.ts index 538baa96e..a37d7d78b 100644 --- a/client/src/components/Planner/useDayDetail.ts +++ b/client/src/components/Planner/useDayDetail.ts @@ -1,10 +1,11 @@ -import { useState, useEffect, useRef } from 'react' +import { useState, useEffect, useEffectEvent, useRef } from 'react' import type { WeatherResult } from '@trek/shared' import { weatherApi } from '../../api/client' import { accommodationRepo } from '../../repo/accommodationRepo' import { isDayInAccommodationRange } from '../../utils/dayOrder' import { applyStayStops } from '../../store/stayStops' import { stayCheckoutDay, stayRequestBody } from './stayFormModel' +import { useStaysRefresh } from './useStaysRefresh' import type { Accommodation, Day } from '../../types' export interface HotelForm { @@ -81,6 +82,23 @@ export function useDayRename(onCommit: (title: string) => void) { return { editingTitle, setEditingTitle, titleDraft, setTitleDraft, titleInputRef, startRename, commitRename } } +/** + * The stay the panel holds once its stays were read again. The picked one (the + * stay being edited) stays picked, as its fresh copy, while it is still on the + * day. One that left the day while the panel stayed on it, cancelled or moved + * away by someone else, leaves nothing picked: falling back to the day's first + * stay would hand an open editor another stay to save over. With nothing + * picked, or on another day, the day's first stay is taken. + */ +function stayAfterReload(picked: Accommodation | null, forDay: Accommodation[], sameDay: boolean): Accommodation | null { + if (picked) { + const kept = forDay.find(a => a.id === picked.id) + if (kept) return kept + if (sameDay) return null + } + return forDay[0] ?? null +} + /** Day-detail data + accommodation logic: weather load, accommodations list, * hotel picker form state and create/update/delete handlers. * @@ -107,21 +125,39 @@ export function useDayDetail(day: Day | null, days: Day[], tripId: number, lat: // would otherwise overwrite the new day's forecast and hotel. const { weather, loading } = useDayForecast(!!(day?.date && lat && lng), day?.date, lat, lng, language) + // A stay booked, moved or cancelled elsewhere (the assistant, a fellow + // traveller, a plugin, a booking import) reaches this panel as a nudge, and the + // panel reads its stays again through the effect below. + const [staysRevision, setStaysRevision] = useState(0) + useStaysRefresh(() => setStaysRevision(r => r + 1)) + + // The day the stays on show were read for, so that a read of the same day can + // tell a stay that left it from a switch to another day. + const staysDayRef = useRef(undefined) + // Shows a read of the stays against the latest pick and editor state. + const showStays = useEffectEvent((all: Accommodation[]) => { + const forDay = all.filter(isForDay) + const sameDay = staysDayRef.current === day?.id + staysDayRef.current = day?.id + const next = stayAfterReload(accommodation, forDay, sameDay) + setAccommodations(all) + setDayAccommodations(forDay) + setAccommodation(next) + // The stay open in the editor is no longer on this day: close the editor, + // so Save neither writes its form over another stay nor books it anew. + if (showHotelPicker === 'edit' && accommodation && next?.id !== accommodation.id) setShowHotelPicker(false) + }) + useEffect(() => { if (!tripId) return let cancelled = false accommodationRepo.list(tripId) .then(data => { - if (cancelled) return - const all: Accommodation[] = data.accommodations || [] - setAccommodations(all) - const allForDay = all.filter(isForDay) - setDayAccommodations(allForDay) - setAccommodation(allForDay[0] || null) + if (!cancelled) showStays(data.accommodations || []) }) .catch(() => {}) return () => { cancelled = true } - }, [tripId, day?.id]) + }, [tripId, day?.id, staysRevision]) useEffect(() => { if (day) setHotelDayRange(defaultHotelDayRange(day)) }, [day?.id]) @@ -154,7 +190,7 @@ export function useDayDetail(day: Day | null, days: Day[], tripId: number, lat: setAccommodations(all) const forDay = all.filter(isForDay) setDayAccommodations(forDay) - setAccommodation(forDay[0] || null) + setAccommodation(stayAfterReload(accommodation, forDay, true)) onAccommodationChange?.() } diff --git a/client/src/components/Planner/useStaysRefresh.test.ts b/client/src/components/Planner/useStaysRefresh.test.ts new file mode 100644 index 000000000..1a47728b0 --- /dev/null +++ b/client/src/components/Planner/useStaysRefresh.test.ts @@ -0,0 +1,127 @@ +// FE-PLANNER-STAYSREFRESH-001 to -007: a stay written elsewhere reaches the +// planner's and the day panel's page-local stays as one reload per burst, and +// never as two reloads at once. +import { act, renderHook } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { resetAllStores } from '../../../tests/helpers/store'; +import { useTripStore } from '../../store/tripStore'; +import { STAYS_REFRESH_SETTLE_MS, useStaysRefresh } from './useStaysRefresh'; + +/** An event from the trip room, as the socket hands it to the store. */ +const remote = (event: { type: string; [key: string]: unknown }) => + act(() => useTripStore.getState().handleRemoteEvent(event)); + +/** Lets the given time pass on the faked clock. */ +const elapse = (ms: number) => + act(() => { + vi.advanceTimersByTime(ms); + }); + +beforeEach(() => { + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); + resetAllStores(); +}); + +describe('useStaysRefresh', () => { + it('FE-PLANNER-STAYSREFRESH-001: a burst of stay events from elsewhere is served by exactly one reload', () => { + const reload = vi.fn(); + renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'accommodation:created', accommodation: { id: 8 } }); + remote({ type: 'accommodation:updated', accommodation: { id: 8 } }); + remote({ type: 'accommodation:deleted', accommodationId: 7 }); + elapse(STAYS_REFRESH_SETTLE_MS - 1); + expect(reload).not.toHaveBeenCalled(); + + elapse(1); + expect(reload).toHaveBeenCalledTimes(1); + elapse(STAYS_REFRESH_SETTLE_MS * 4); + expect(reload).toHaveBeenCalledTimes(1); + }); + + it('FE-PLANNER-STAYSREFRESH-002: an event that cannot touch a stay asks for nothing', () => { + const reload = vi.fn(); + renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'reservation:deleted', reservationId: 3 }); + remote({ type: 'todo:deleted', itemId: 4 }); + elapse(STAYS_REFRESH_SETTLE_MS * 2); + expect(reload).not.toHaveBeenCalled(); + }); + + it('FE-PLANNER-STAYSREFRESH-003: a stay event after the reload went out starts the next one', () => { + const reload = vi.fn(); + renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'accommodation:created', accommodation: { id: 8 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + remote({ type: 'accommodation:created', accommodation: { id: 9 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + expect(reload).toHaveBeenCalledTimes(2); + }); + + it('FE-PLANNER-STAYSREFRESH-004: the reload handed in last is the one that runs', () => { + const first = vi.fn(); + const second = vi.fn(); + const { rerender } = renderHook(({ reload }) => useStaysRefresh(reload), { initialProps: { reload: first } }); + + remote({ type: 'accommodation:updated', accommodation: { id: 8 } }); + rerender({ reload: second }); + elapse(STAYS_REFRESH_SETTLE_MS); + expect(first).not.toHaveBeenCalled(); + expect(second).toHaveBeenCalledTimes(1); + }); + + it('FE-PLANNER-STAYSREFRESH-005: unmounting drops a pending reload and stops listening', () => { + const reload = vi.fn(); + const { unmount } = renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'accommodation:created', accommodation: { id: 8 } }); + unmount(); + remote({ type: 'accommodation:created', accommodation: { id: 9 } }); + elapse(STAYS_REFRESH_SETTLE_MS * 2); + expect(reload).not.toHaveBeenCalled(); + }); + + it('FE-PLANNER-STAYSREFRESH-006: a burst that ends while a reload reads waits for it, then gets one more', async () => { + let land: () => void = () => {}; + const reload = vi.fn( + () => + new Promise((resolve) => { + land = resolve; + }) + ); + renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'accommodation:created', accommodation: { id: 8 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + remote({ type: 'accommodation:updated', accommodation: { id: 8 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + remote({ type: 'accommodation:deleted', accommodationId: 8 }); + elapse(STAYS_REFRESH_SETTLE_MS); + expect(reload).toHaveBeenCalledTimes(1); + + await act(async () => land()); + expect(reload).toHaveBeenCalledTimes(2); + await act(async () => land()); + elapse(STAYS_REFRESH_SETTLE_MS * 4); + expect(reload).toHaveBeenCalledTimes(2); + }); + + it('FE-PLANNER-STAYSREFRESH-007: a reload that fails does not hold back the next burst', async () => { + const reload = vi.fn(() => Promise.reject(new Error('offline'))); + renderHook(() => useStaysRefresh(reload)); + + remote({ type: 'accommodation:created', accommodation: { id: 8 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + await act(async () => {}); + remote({ type: 'accommodation:created', accommodation: { id: 9 } }); + elapse(STAYS_REFRESH_SETTLE_MS); + expect(reload).toHaveBeenCalledTimes(2); + }); +}); diff --git a/client/src/components/Planner/useStaysRefresh.ts b/client/src/components/Planner/useStaysRefresh.ts new file mode 100644 index 000000000..cddd7b811 --- /dev/null +++ b/client/src/components/Planner/useStaysRefresh.ts @@ -0,0 +1,76 @@ +import { useEffect, useEffectEvent } from 'react'; + +/** + * How long refresh nudges are collected before the stays are read once. The + * same window `useServerPing` folds a burst into: long enough for an assistant + * step that books several stays at once, short enough that one booking still + * reads as instant. + */ +export const STAYS_REFRESH_SETTLE_MS = 250; + +/** + * Runs `reload` once for each burst of `accommodations:refresh` nudges. + * + * The stays live in page-local state (the planner's list, the desktop day + * panel's own copy), so the trip store cannot apply a remote stay to them. Instead + * `remoteEventHandler` and the trip store nudge the views on every change that + * can add, move or take away a booked night, and each view reads its stays again. + * One assistant step can apply up to fifty changes, every one of them its own + * event, and the planner's reload reads the reservations too, so the nudges of + * one burst are served by a single reload instead of one each. + * + * As in `useServerPing`, two reloads never overlap: a burst that ends while a + * reload is still reading waits for it and is then served by one more, so an + * older answer cannot land over a newer one. A `reload` that returns nothing + * counts as done at once. + * + * The nudge rides the legacy window event the store already sends; this hook is + * its one listener. Moving the stays into a store slice would retire both. + * + * `reload` may change identity every render; the latest one is always used. + */ +export function useStaysRefresh(reload: () => Promise | void): void { + const onSettled = useEffectEvent(reload); + + useEffect(() => { + let timer: ReturnType | null = null; + let inFlight = false; + /** A burst that ended while a reload was reading: served once that lands. */ + let pendingAfterFlight = false; + let cancelled = false; + + const landed = (): void => { + inFlight = false; + if (pendingAfterFlight && !cancelled) { + pendingAfterFlight = false; + run(); + } + }; + + const run = (): void => { + if (cancelled) return; + if (inFlight) { + pendingAfterFlight = true; + return; + } + const reading = onSettled(); + if (!(reading instanceof Promise)) return; // done at once + inFlight = true; + void reading.then(landed, landed); + }; + + const collect = (): void => { + if (timer) return; // already collecting this burst + timer = setTimeout(() => { + timer = null; + run(); + }, STAYS_REFRESH_SETTLE_MS); + }; + window.addEventListener('accommodations:refresh', collect); + return () => { + cancelled = true; + window.removeEventListener('accommodations:refresh', collect); + if (timer) clearTimeout(timer); + }; + }, []); +} diff --git a/client/src/components/Planner/useTripStays.test.ts b/client/src/components/Planner/useTripStays.test.ts new file mode 100644 index 000000000..ac39ee030 --- /dev/null +++ b/client/src/components/Planner/useTripStays.test.ts @@ -0,0 +1,92 @@ +// FE-PLANNER-TRIPSTAYS-001 to -004: the planner's stays, read again for a stay +// written elsewhere, with the newest read winning over an older one. +import { act, renderHook, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { resetAllStores } from '../../../tests/helpers/store'; +import { accommodationRepo } from '../../repo/accommodationRepo'; +import { useTripStore } from '../../store/tripStore'; +import type { Accommodation } from '../../types'; +import { useTripStays } from './useTripStays'; + +type StayList = Awaited>; + +const stay = (id: number): Accommodation => ({ id, trip_id: 3, place_id: 7, start_day_id: 1, end_day_id: 2 }); +const answer = (...ids: number[]): StayList => ({ accommodations: ids.map(stay) }); + +/** A read whose answer the test hands over when it chooses. */ +function deferredRead() { + let settle: (value: StayList) => void = () => {}; + const promise = new Promise((resolve) => { + settle = resolve; + }); + return { promise, settle }; +} + +let loadReservations: ReturnType Promise>>; + +beforeEach(() => { + loadReservations = vi.fn(async () => {}); + useTripStore.setState({ loadReservations }); +}); + +afterEach(() => { + vi.restoreAllMocks(); + resetAllStores(); +}); + +describe('useTripStays', () => { + it('FE-PLANNER-TRIPSTAYS-001: a burst of stays booked elsewhere reads the stays and their bookings once more', async () => { + const list = vi.spyOn(accommodationRepo, 'list').mockResolvedValueOnce(answer()).mockResolvedValue(answer(8, 9)); + const { result } = renderHook(() => useTripStays(3)); + act(() => result.current.loadAccommodations()); + await waitFor(() => expect(list).toHaveBeenCalledTimes(1)); + + act(() => { + useTripStore.getState().handleRemoteEvent({ type: 'accommodation:created', accommodation: stay(8) }); + useTripStore.getState().handleRemoteEvent({ type: 'accommodation:created', accommodation: stay(9) }); + }); + + await waitFor(() => expect(result.current.tripAccommodations.map((a) => a.id)).toEqual([8, 9])); + expect(list).toHaveBeenCalledTimes(2); + expect(loadReservations).toHaveBeenCalledTimes(2); + expect(loadReservations).toHaveBeenLastCalledWith(3); + }); + + it('FE-PLANNER-TRIPSTAYS-002: an older answer that lands last does not overwrite the newer one', async () => { + const older = deferredRead(); + const newer = deferredRead(); + vi.spyOn(accommodationRepo, 'list').mockReturnValueOnce(older.promise).mockReturnValueOnce(newer.promise); + const { result } = renderHook(() => useTripStays(3)); + + act(() => result.current.loadAccommodations()); + act(() => result.current.loadAccommodations()); + await act(async () => newer.settle(answer(8, 9))); + await act(async () => older.settle(answer(8))); + + expect(result.current.tripAccommodations.map((a) => a.id)).toEqual([8, 9]); + }); + + it('FE-PLANNER-TRIPSTAYS-003: a read that fails keeps the stays on show', async () => { + const list = vi.spyOn(accommodationRepo, 'list').mockResolvedValueOnce(answer(8)); + const { result } = renderHook(() => useTripStays(3)); + act(() => result.current.loadAccommodations()); + await waitFor(() => expect(result.current.tripAccommodations.map((a) => a.id)).toEqual([8])); + + list.mockRejectedValueOnce(new Error('offline')); + await act(async () => result.current.loadAccommodations()); + + expect(list).toHaveBeenCalledTimes(2); + expect(result.current.tripAccommodations.map((a) => a.id)).toEqual([8]); + }); + + it('FE-PLANNER-TRIPSTAYS-004: without a trip id nothing is read', () => { + const list = vi.spyOn(accommodationRepo, 'list'); + const { result } = renderHook(() => useTripStays(Number.NaN)); + + act(() => result.current.loadAccommodations()); + + expect(list).not.toHaveBeenCalled(); + expect(loadReservations).not.toHaveBeenCalled(); + }); +}); diff --git a/client/src/components/Planner/useTripStays.ts b/client/src/components/Planner/useTripStays.ts new file mode 100644 index 000000000..0f7e060ad --- /dev/null +++ b/client/src/components/Planner/useTripStays.ts @@ -0,0 +1,46 @@ +import { useCallback, useRef, useState } from 'react'; + +import { accommodationRepo } from '../../repo/accommodationRepo'; +import { useTripStore } from '../../store/tripStore'; +import type { Accommodation } from '../../types'; +import { useStaysRefresh } from './useStaysRefresh'; + +/** + * The planner's stays: the trip's whole list, which the sidebar, the map and the + * phone's day sheet read. They live here rather than in the trip store, so a stay + * written elsewhere (the assistant, a fellow traveller, a plugin, a booking + * import) reaches them as a nudge, and `useStaysRefresh` folds a burst of nudges + * into one read. + * + * A read takes the stays' bookings along, since a stay is booked as a hotel + * reservation. The newest read wins: the read after an own write and the one for + * a stay written elsewhere can be on their way at once, and the older answer must + * not land over the newer one. + */ +export function useTripStays(tripId: number) { + const [tripAccommodations, setTripAccommodations] = useState([]); + const latestRead = useRef(0); + + const readStays = useCallback(async () => { + if (!tripId) return; + const read = ++latestRead.current; + await Promise.all([ + accommodationRepo.list(tripId).then( + (d) => { + if (read === latestRead.current) setTripAccommodations(d.accommodations || []); + }, + // A read that fails keeps the stays on show; the next nudge reads them again. + () => {} + ), + useTripStore.getState().loadReservations(tripId), + ]); + }, [tripId]); + + const loadAccommodations = useCallback(() => { + void readStays(); + }, [readStays]); + + useStaysRefresh(readStays); + + return { tripAccommodations, setTripAccommodations, loadAccommodations }; +} diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index b8ca25d26..1eeac1c1a 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -341,16 +341,16 @@ describe('useTripPlanner — bootstrap', () => { expect(actions.loadReservations).toHaveBeenCalledTimes(1) }) - it('FE-TP-HOOK-011: the accommodations:refresh event reloads the accommodation list', async () => { + it('FE-TP-HOOK-011: stays booked elsewhere in one burst reload the stay list once and show in it', async () => { seedTrip() - await renderPlanner() - await waitFor(() => expect(accommodationRepo.list).toHaveBeenCalledTimes(1)) + const { result } = await renderPlanner() + const booked = [8, 9].map(id => ({ id, trip_id: 42 })) + vi.mocked(accommodationRepo.list).mockResolvedValue({ accommodations: booked as never }) - await act(async () => { - window.dispatchEvent(new Event('accommodations:refresh')) - }) - - await waitFor(() => expect(accommodationRepo.list).toHaveBeenCalledTimes(2)) + act(() => booked.forEach(accommodation => + useTripStore.getState().handleRemoteEvent({ type: 'accommodation:created', accommodation }))) + await waitFor(() => expect(result.current.tripAccommodations.map(a => a.id)).toEqual([8, 9])) + expect(accommodationRepo.list).toHaveBeenCalledTimes(2) }) it('FE-TP-HOOK-012: the splash gate opens once the trip finished loading', async () => { diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 0ebf7669f..c0332d558 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -12,7 +12,6 @@ import { resolvePluginIcon } from '../../components/shared/PluginIcon' import { useTranslation } from '../../i18n' import { addonsApi, authApi, tripsApi, healthApi, placesApi } from '../../api/client' import { TRANSPORT_TYPES } from '../../utils/dayMerge' -import { accommodationRepo } from '../../repo/accommodationRepo' import { offlineDb } from '../../db/offlineDb' import { isEffectivelyOffline } from '../../sync/networkMode' import { useAuthStore } from '../../store/authStore' @@ -32,13 +31,14 @@ import { useAddonStore } from '../../store/addonStore' import { usePlannerHistory } from '../../hooks/usePlannerHistory' import { useIsTouch } from '../../hooks/useIsTouch' import { usePluginStore } from '../../store/pluginStore' -import type { Accommodation, Assignment, TripMember, Day, Reservation } from '../../types' +import type { Assignment, TripMember, Day, Reservation } from '../../types' import { OFM_POSITRON, DEFAULT_MAP_LAT, DEFAULT_MAP_LNG, DEFAULT_MAP_ZOOM } from '../../constants/mapDefaults' import { useTileUrl } from '../../hooks/useTileUrl' import { placesForDays } from './tripPlannerModel' import { isDeepLinkableTripTab, TRIP_TAB_LABEL_KEYS } from '../../constants/tripTabs' import { isRoutableReservation } from '../../utils/reservationRoutes' import { showReservationOnMap } from '../../components/Planner/bookings/showOnMap' +import { useTripStays } from '../../components/Planner/useTripStays' import { parseStoredConnections, resolveEffectiveConnections, resolveVisibleConnectionIds, toggleConnectionId, toggleAllConnections as flipAllConnectionsMode, @@ -161,7 +161,10 @@ export function useTripPlanner() { ) }, [roadtripMode, roadtripSettings.roadtrip_service_stops_in_days, storedAssignments]) const [collabFeatures, setCollabFeatures] = useState<{ chat: boolean; notes: boolean; links: boolean; polls: boolean; whatsnext: boolean }>({ chat: true, notes: true, links: true, polls: true, whatsnext: true }) - const [tripAccommodations, setTripAccommodations] = useState([]) + // Stays live in page-local state rather than the store, so store-level + // refreshes (a stay booked or changed elsewhere, a remote trip date change, + // reconnect hydration) nudge them, one read per burst (#1288). + const { tripAccommodations, setTripAccommodations, loadAccommodations } = useTripStays(tripId) const places = useMemo( () => (roadtripMode ? allPlaces @@ -181,13 +184,6 @@ export function useTripPlanner() { }).catch(() => {}) }, [tripId]) - const loadAccommodations = useCallback(() => { - if (tripId) { - accommodationRepo.list(tripId).then(d => setTripAccommodations(d.accommodations || [])).catch(() => {}) - tripActions.loadReservations(tripId) - } - }, [tripId]) - useEffect(() => { addonsApi.enabled().then(data => { const map: Record = {} @@ -424,14 +420,6 @@ export function useTripPlanner() { } }, [tripId]) - // Accommodations live in this hook's local state, so store-level refreshes - // (remote trip date change, reconnect hydration) nudge us via this event (#1288). - useEffect(() => { - const onRefresh = () => loadAccommodations() - window.addEventListener('accommodations:refresh', onRefresh) - return () => window.removeEventListener('accommodations:refresh', onRefresh) - }, [loadAccommodations]) - useTripWebSocket(tripId, invalidateTourPlaceIds) // While the assistant compares a block it changed with the plan before it, the map shows that plan. diff --git a/client/src/store/slices/remoteEventHandler.ts b/client/src/store/slices/remoteEventHandler.ts index 4a912c986..91d4aa12a 100644 --- a/client/src/store/slices/remoteEventHandler.ts +++ b/client/src/store/slices/remoteEventHandler.ts @@ -529,8 +529,18 @@ export const STATE_APPLIERS: Partial> }, } -/** The deletes that can take a booked night along with them. */ -const STAY_TAKING_EVENTS = new Set(['day:deleted', 'place:deleted', 'accommodation:deleted']) +/** + * The changes that add, move or take away a booked night: a stay written + * elsewhere (the assistant, a fellow traveller, a plugin, a booking import), and + * the deletes that can take a stay along with them. + */ +const STAY_REFRESH_EVENTS = new Set([ + 'accommodation:created', + 'accommodation:updated', + 'accommodation:deleted', + 'day:deleted', + 'place:deleted', +]) /** * Applies a remote WebSocket event to the local Zustand store, keeping state in sync across collaborators. @@ -574,13 +584,15 @@ export function handleRemoteEvent(set: SetState, get: GetState, event: WebSocket if (tripId) get().loadReservations(tripId) } - // A deleted day cancels a stay that checked in or out on it, a deleted place - // the stay booked at it, and the stays live in page-local planner state rather - // than this store. This rides the planner's existing accommodations:refresh - // event on purpose, the legacy window bus the place image and trip date - // handlers here use as well. Retiring it means moving the stays into a store - // slice, a change of its own. - if (STAY_TAKING_EVENTS.has(type)) window.dispatchEvent(new CustomEvent('accommodations:refresh')) + // A stay booked, changed or cancelled elsewhere, a deleted day that cancels a + // stay checking in or out on it, a deleted place that takes the stay booked at + // it: the stays live in page-local state (useTripPlanner, useDayDetail) rather + // than this store, so each of these asks them to read their stays again, and + // useStaysRefresh folds a burst into one reload. This rides the planner's + // existing accommodations:refresh event on purpose, the legacy window bus the + // place image and trip date handlers here use as well. Retiring it means + // moving the stays into a store slice, a change of its own. + if (STAY_REFRESH_EVENTS.has(type)) window.dispatchEvent(new CustomEvent('accommodations:refresh')) // A reorder/insert re-pins dates and re-stamps booking times server-side, so // pull the authoritative days + reservations for collaborators. diff --git a/client/tests/unit/remoteEventHandler/accommodations.test.ts b/client/tests/unit/remoteEventHandler/accommodations.test.ts new file mode 100644 index 000000000..f955c6262 --- /dev/null +++ b/client/tests/unit/remoteEventHandler/accommodations.test.ts @@ -0,0 +1,52 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useTripStore } from '../../../src/store/tripStore'; +import { buildReservation } from '../../helpers/factories'; +import { resetAllStores } from '../../helpers/store'; + +/** + * Stays live in page-local planner state, so a stay written elsewhere (the + * assistant, a fellow traveller, a plugin, a booking import) cannot be applied + * to the store. The handler asks the planner to read them again instead; the + * planner's useStaysRefresh folds the asks of a burst into one reload. + */ +describe('remoteEventHandler > accommodations', () => { + const refresh = vi.fn(); + + beforeEach(() => { + resetAllStores(); + refresh.mockClear(); + window.addEventListener('accommodations:refresh', refresh); + }); + + afterEach(() => { + window.removeEventListener('accommodations:refresh', refresh); + }); + + it.each([ + ['accommodation:created', { accommodation: { id: 8, trip_id: 1 } }], + ['accommodation:created', {}], + ['accommodation:updated', { accommodation: { id: 8, trip_id: 1 } }], + ['accommodation:deleted', { accommodationId: 8 }], + ])('FE-WSEVT-STAY-001: %s from elsewhere asks the planner once to reload its stays', (type, payload) => { + const reservations = [buildReservation({ id: 3 })]; + useTripStore.setState({ reservations }); + + useTripStore.getState().handleRemoteEvent({ type, ...payload }); + + expect(refresh).toHaveBeenCalledTimes(1); + expect(useTripStore.getState().reservations).toBe(reservations); + }); + + it('FE-WSEVT-STAY-002: a booking change that is no stay leaves the stays alone', () => { + const reservation = buildReservation({ id: 3 }); + useTripStore.setState({ reservations: [reservation] }); + + useTripStore + .getState() + .handleRemoteEvent({ type: 'reservation:updated', reservation: { ...reservation, title: 'Moved' } }); + useTripStore.getState().handleRemoteEvent({ type: 'reservation:deleted', reservationId: 3 }); + + expect(refresh).not.toHaveBeenCalled(); + }); +});