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(); + }); +});