From 61d4fd4d641f94e8e091185a3c691c7f07bd51eb Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 23 Aug 2026 10:07:43 +0200 Subject: [PATCH] fix(planner): scheme-check the booking URL, and stop the serial routing calls A booking's url is rendered as a plain href to every other member of the trip and nothing ever checked its scheme, so a collaborator could store javascript: there. The place website next to it has been going through safeHttpUrl for exactly this reason, with a comment saying stored values predate the server check - this is the surface that was left. Both ends now: safeHttpUrl at the render site, and the scheme in the shared contract so nothing new gets stored, additively so existing rows still load. The per-leg routing rewrite turned one multi-waypoint request per run into one serial request per leg. A twelve-stop day is eleven round trips to a public OSRM in sequence before a single connector appears, and toggling km/mi does it all again. Group consecutive legs that resolve to the same mode into one request and fire the groups concurrently, keeping leg order on the join. Transport reordering wrote to the store and persisted with a bare catch, so a failed PUT left the sidebar showing an order the server never got; it reconciles now. Plus the smaller ones: the OSM menu_url rendered unchecked, expand/collapse-all persisting to a different storage than the day list reads, the debounced address search with no ordering guard, the accommodation save chain with no error path, and window.__trek_map holding a destroyed map. --- client/src/components/Map/MapViewGL.test.tsx | 9 ++ client/src/components/Map/MapViewGL.tsx | 5 ++ .../components/Planner/AddressInput.test.tsx | 24 +++++- .../src/components/Planner/AddressInput.tsx | 14 +++- .../Planner/DayDetailPanel.test.tsx | 28 ++++++- .../src/components/Planner/DayDetailPanel.tsx | 69 +++++++++------- .../Planner/DayPlanSidebar.test.tsx | 71 ++++++++++++++++ .../src/components/Planner/DayPlanSidebar.tsx | 82 +++++++++++++++---- .../Planner/DayPlanSidebarToolbar.test.tsx | 8 +- .../Planner/DayPlanSidebarToolbar.tsx | 4 +- .../Planner/PlaceDetailsColumn.test.tsx | 14 ++++ .../components/Planner/PlaceDetailsColumn.tsx | 8 +- .../Planner/ReservationsPanel.test.tsx | 19 ++++- .../components/Planner/ReservationsPanel.tsx | 12 ++- client/src/hooks/useRouteCalculation.ts | 19 +++-- .../hooks/useRouteCalculation.test.ts | 42 ++++++++++ .../reservation/reservation.schema.spec.ts | 18 ++++ shared/src/reservation/reservation.schema.ts | 24 +++++- 18 files changed, 407 insertions(+), 63 deletions(-) diff --git a/client/src/components/Map/MapViewGL.test.tsx b/client/src/components/Map/MapViewGL.test.tsx index eceae5a2f..0b9de7f13 100644 --- a/client/src/components/Map/MapViewGL.test.tsx +++ b/client/src/components/Map/MapViewGL.test.tsx @@ -1696,6 +1696,15 @@ describe('MapViewGL', () => { expect(glMap.remove).toHaveBeenCalled() }) + it('FE-COMP-MAPVIEWGL-069: unmounting lets go of the window debug handle', async () => { + const { unmount } = render() + await act(async () => {}) + expect((window as any).__trek_map).toBe(glMap) + unmount() + // Otherwise every style switch keeps its torn-down map reachable from window. + expect((window as any).__trek_map).toBeUndefined() + }) + it('FE-COMP-MAPVIEWGL-059: pan and zoom report the visible bbox for the POI explore pill', async () => { const onViewportChange = vi.fn() render() diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index eeac19e91..a2f3ab69a 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -930,6 +930,11 @@ export function MapViewGL({ } try { map.remove() } catch { /* noop */ } mapRef.current = null + // Drop the debug handle too, or a style switch keeps every torn-down map + // (canvas and sources included) alive for the rest of the page's life. The + // identity check leaves a freshly built map alone if this cleanup runs late. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if ((window as any).__trek_map === map) delete (window as any).__trek_map setMapReady(false) } }, [glProvider, glStyle, mapboxToken, enableMapbox3d, mapboxQuality]) // rebuild on provider/style changes only diff --git a/client/src/components/Planner/AddressInput.test.tsx b/client/src/components/Planner/AddressInput.test.tsx index e52d93c7c..617305d7f 100644 --- a/client/src/components/Planner/AddressInput.test.tsx +++ b/client/src/components/Planner/AddressInput.test.tsx @@ -1,4 +1,4 @@ -// FE-PLANNER-ADDRIN-001 to FE-PLANNER-ADDRIN-014 +// FE-PLANNER-ADDRIN-001 to FE-PLANNER-ADDRIN-015 import { useState } from 'react'; import { delay, http, HttpResponse } from 'msw'; import userEvent from '@testing-library/user-event'; @@ -206,6 +206,28 @@ describe('AddressInput', () => { expect(input).toHaveValue('Sacher'); }); + it('FE-PLANNER-ADDRIN-015: a slow earlier search cannot overwrite the newer results', async () => { + // The debounce only cancels the timer; a request already on its way keeps + // going, and this one answers last. + server.use(http.post('/api/maps/search', async ({ request }) => { + const { query } = await request.json() as { query: string }; + if (query === 'Sacher') { await delay(600); return HttpResponse.json({ places: [HOTEL] }); } + return HttpResponse.json({ places: [CAFE] }); + })); + + render(); + const input = screen.getByPlaceholderText('Street and number'); + fireEvent.change(input, { target: { value: 'Sacher' } }); + await settle(350); + fireEvent.change(input, { target: { value: 'Cafe Central' } }); + + expect(await screen.findByText('Cafe Central')).toBeInTheDocument(); + // Now let the first request land. + await settle(500); + expect(screen.getByText('Cafe Central')).toBeInTheDocument(); + expect(screen.queryByText('Hotel Sacher')).not.toBeInTheDocument(); + }); + it('FE-PLANNER-ADDRIN-014: a mousedown outside closes the list and a failing search empties it', async () => { const user = userEvent.setup(); server.use(searchRoute([HOTEL])); diff --git a/client/src/components/Planner/AddressInput.tsx b/client/src/components/Planner/AddressInput.tsx index 07c51f0f3..50ed98b32 100644 --- a/client/src/components/Planner/AddressInput.tsx +++ b/client/src/components/Planner/AddressInput.tsx @@ -22,6 +22,9 @@ export default function AddressInput({ value, onChange, placeholder, className } const [loading, setLoading] = useState(false) const wrapRef = useRef(null) const debounceRef = useRef | null>(null) + // Clearing the timer does nothing to a request that is already out, and + // mapsApi.search takes no signal, so results are matched by ticket instead. + const reqIdRef = useRef(0) useEffect(() => { const handler = (e: MouseEvent) => { @@ -38,23 +41,28 @@ export default function AddressInput({ value, onChange, placeholder, className } const search = (text: string) => { if (debounceRef.current) clearTimeout(debounceRef.current) const trimmed = text.trim() - if (trimmed.length < 3) { setResults([]); setLoading(false); return } + // Same reason as in pick(): a request for the longer text may still land. + if (trimmed.length < 3) { reqIdRef.current++; setResults([]); setLoading(false); return } debounceRef.current = setTimeout(async () => { + const myReq = ++reqIdRef.current setLoading(true) try { const data = await mapsApi.search(trimmed, locale) + if (myReq !== reqIdRef.current) return setResults(data.places || []) setHighlight(-1) } catch { - setResults([]) + if (myReq === reqIdRef.current) setResults([]) } finally { - setLoading(false) + if (myReq === reqIdRef.current) setLoading(false) } }, 320) } const pick = (r: any) => { if (debounceRef.current) clearTimeout(debounceRef.current) + // A response still on its way must not repopulate the list behind the pick. + reqIdRef.current++ onChange(r.address || r.name || '') setOpen(false) setResults([]) diff --git a/client/src/components/Planner/DayDetailPanel.test.tsx b/client/src/components/Planner/DayDetailPanel.test.tsx index d1f84e3e4..f8ee30de8 100644 --- a/client/src/components/Planner/DayDetailPanel.test.tsx +++ b/client/src/components/Planner/DayDetailPanel.test.tsx @@ -1,4 +1,4 @@ -// FE-PLANNER-DAYDETAIL-001 to FE-PLANNER-DAYDETAIL-079 +// FE-PLANNER-DAYDETAIL-001 to FE-PLANNER-DAYDETAIL-080 import React from 'react'; import { fireEvent, render, screen, waitFor, within, act } from '../../../tests/helpers/render'; import userEvent from '@testing-library/user-event'; @@ -342,6 +342,32 @@ describe('DayDetailPanel', () => { expect(buttons.length).toBeGreaterThanOrEqual(2); }); + it('FE-PLANNER-DAYDETAIL-080: a failing accommodation edit keeps the picker open and says so', async () => { + const addToast = vi.fn(); + window.__addToast = addToast; + server.use( + http.get('/api/trips/1/accommodations', () => + HttpResponse.json({ + accommodations: [{ + id: 1, place_id: 5, place_name: 'Grand Hotel', place_address: 'Paris', + start_day_id: 1, end_day_id: 3, check_in: '14:00', check_out: null, confirmation: null, + }], + }), + ), + http.put('/api/trips/1/accommodations/1', () => HttpResponse.json({ error: 'Stay overlaps' }, { status: 400 })), + ); + render(); + await screen.findByText('Grand Hotel'); + // The pencil beside the stay opens the picker in edit mode. + await userEvent.click(document.querySelector('.lucide-pencil')!.closest('button')!); + const picker = await waitFor(() => document.body.querySelector('[style*="z-index: 99999"]') as HTMLElement); + await userEvent.click(within(picker).getByText('Save')); + + await waitFor(() => expect(addToast).toHaveBeenCalledWith('Stay overlaps', 'error', undefined)); + // The picker stays put so the entered values are not lost. + expect(document.body.querySelector('[style*="z-index: 99999"]')).toBeInTheDocument(); + }); + it('FE-PLANNER-DAYDETAIL-022: accommodation edit/remove buttons hidden when canEditDays=false', async () => { // Use regular user + restrict day_edit to admin only const regularUser = buildUser({ id: 999, role: 'user' }); diff --git a/client/src/components/Planner/DayDetailPanel.tsx b/client/src/components/Planner/DayDetailPanel.tsx index e430ad24b..39eabef90 100644 --- a/client/src/components/Planner/DayDetailPanel.tsx +++ b/client/src/components/Planner/DayDetailPanel.tsx @@ -13,6 +13,7 @@ import { useTripStore } from '../../store/tripStore' import CustomSelect from '../shared/CustomSelect' import CustomTimePicker from '../shared/CustomTimePicker' import { useSettingsStore } from '../../store/settingsStore' +import { useToast } from '../shared/Toast' import { getLocaleForLanguage, useTranslation } from '../../i18n' import type { Day, Place, Category, Reservation, AssignmentsMap } from '../../types' import { isDayInAccommodationRange } from '../../utils/dayOrder' @@ -492,6 +493,44 @@ function HotelPickerModal({ showHotelPicker, setShowHotelPicker, font, t, hotelD days, locale, hotelForm, setHotelForm, categories, hotelCategoryFilter, setHotelCategoryFilter, places, handleSelectPlace, accommodation, tripId, day, setAccommodations, setDayAccommodations, setAccommodation, handleSaveAccommodation, onAccommodationChange }: any) { + const toast = useToast() + // Saving the picker. The edit branch stays here rather than in useDayDetail + // because it also refreshes the panel's own accommodation state; it has to say + // what went wrong when the write fails, otherwise the picker just sits there + // with the Save button doing nothing. + const saveHotelPicker = async () => { + if (showHotelPicker !== 'edit' || !accommodation) { + await handleSaveAccommodation() + return + } + try { + await accommodationsApi.update(tripId, accommodation.id, { + place_id: hotelForm.place_id, + start_day_id: hotelDayRange.start, + end_day_id: hotelDayRange.end, + check_in: hotelForm.check_in || null, + check_in_end: hotelForm.check_in_end || null, + check_out: hotelForm.check_out || null, + confirmation: hotelForm.confirmation || null, + }) + setShowHotelPicker(false) + setHotelForm({ check_in: '', check_in_end: '', check_out: '', confirmation: '', place_id: null }) + // Reload + const d = await accommodationsApi.list(tripId) + const all = d.accommodations || [] + setAccommodations(all) + setDayAccommodations(all.filter(a => + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false + )) + const acc = all.find(a => day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false) + setAccommodation(acc || null) + onAccommodationChange?.() + } catch (err: unknown) { + const message = (err as { response?: { data?: { error?: string } } })?.response?.data?.error + toast.error(message || t('common.unknownError')) + } + } + return ( <> {/* Hotel Picker Popup — portal to body to escape transform stacking context */} @@ -635,35 +674,7 @@ function HotelPickerModal({ showHotelPicker, setShowHotelPicker, font, t, hotelD -