From 3e532aab99e5a26518d5da9130e7600846d0c984 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 5 Sep 2026 12:39:22 +0200 Subject: [PATCH] fix(planner): never link an offline temp place id to a hotel booking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The envelope fix routes the created place through the store, which offline mints a NEGATIVE temp id: reservations have no offline write path, the mutation queue rewrites temp ids in a URL but never inside another entity's body, and day_accommodations.place_id carries a foreign key. So a forced-offline save (the Settings toggle sets the flag while the network is still live) reached the server with place_id: -1758..., failed the FK inside the insert transaction and rolled the whole booking back into a 500 — where dev merely saved it unlinked. Worse, the temp place is in `places` by then, so every retry matched the same negative id and the booking stayed unsavable for the rest of the session. Offline the function now links nothing at all and skips the geocode too: the booking cannot be written either way, so minting a place would only leave an orphan behind on the next flush. The name-match arm rejects temp ids for the same reason. `as never` is gone from the addPlace argument, and placesApi.create finally declares its `{ place }` return type — an untyped `r.data` is what let the envelope be misread in the first place. --- client/src/api/client.ts | 5 ++- .../pages/tripPlanner/useTripPlanner.test.tsx | 36 +++++++++++++++++++ .../src/pages/tripPlanner/useTripPlanner.ts | 16 +++++++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/client/src/api/client.ts b/client/src/api/client.ts index 871db664f..cf6cdc845 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -432,7 +432,10 @@ export const daysApi = { export const placesApi = { list: (tripId: number | string, params?: Record) => apiClient.get(`/trips/${tripId}/places`, { params }).then(r => r.data), - create: (tripId: number | string, data: PlaceCreateRequest) => apiClient.post(`/trips/${tripId}/places`, data).then(r => r.data), + // Typed: an untyped `r.data` is what let `{ place }` be read as a bare place, + // so every hotel booking minted an unlinked duplicate (#2243). + create: (tripId: number | string, data: PlaceCreateRequest): Promise<{ place: Place }> => + apiClient.post(`/trips/${tripId}/places`, data).then(r => r.data), get: (tripId: number | string, id: number | string) => apiClient.get(`/trips/${tripId}/places/${id}`).then(r => r.data), update: (tripId: number | string, id: number | string, data: PlaceUpdateRequest) => apiClient.put(`/trips/${tripId}/places/${id}`, data).then(r => r.data), delete: (tripId: number | string, id: number | string) => apiClient.delete(`/trips/${tripId}/places/${id}`).then(r => r.data), diff --git a/client/src/pages/tripPlanner/useTripPlanner.test.tsx b/client/src/pages/tripPlanner/useTripPlanner.test.tsx index be200d2ed..b4b4bdaad 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.test.tsx +++ b/client/src/pages/tripPlanner/useTripPlanner.test.tsx @@ -1603,6 +1603,42 @@ describe('useTripPlanner — bookings and transports', () => { expect(second.create_accommodation.place_id).toBe(901) }) + it('FE-TP-HOOK-081d: offline, no place is minted for a booking that cannot be written', async () => { + env.forcedOffline = true + seedTrip() + + const { result } = await renderPlanner() + await act(async () => { + await result.current.handleSaveReservation({ + title: 'Ryokan Sakura', type: 'hotel', + create_accommodation: { venue: { name: 'Ryokan Sakura' } }, + } as never) + }) + + // A place created offline gets a negative temp id, and the reservation write + // is online-only — linking one is a foreign-key failure on the server. + expect(actions.addPlace).not.toHaveBeenCalled() + expect(mapsApi.search).not.toHaveBeenCalled() + const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number } } + expect(payload.create_accommodation.place_id).toBeUndefined() + }) + + it('FE-TP-HOOK-081e: a place still holding an offline temp id is not linked as the accommodation', async () => { + actions.addPlace.mockResolvedValue(buildPlace({ id: 902, name: 'Ryokan Sakura' })) + seedTrip({ places: [buildPlace({ id: -1758000000000, name: 'Ryokan Sakura' })] }) + + const { result } = await renderPlanner() + await act(async () => { + await result.current.handleSaveReservation({ + title: 'Ryokan Sakura', type: 'hotel', + create_accommodation: { venue: { name: 'Ryokan Sakura' } }, + } as never) + }) + + const payload = actions.addReservation.mock.calls[0][1] as { create_accommodation: { place_id?: number } } + expect(payload.create_accommodation.place_id).toBe(902) + }) + it('FE-TP-HOOK-081b: a geocoded venue without a reviewed address adopts the hit address', async () => { vi.mocked(mapsApi.search).mockResolvedValue({ places: [{ lat: 35.1, lng: 135.7, address: 'Kyoto, JP' }], diff --git a/client/src/pages/tripPlanner/useTripPlanner.ts b/client/src/pages/tripPlanner/useTripPlanner.ts index 753a8d0f3..a6b538fad 100644 --- a/client/src/pages/tripPlanner/useTripPlanner.ts +++ b/client/src/pages/tripPlanner/useTripPlanner.ts @@ -916,8 +916,18 @@ export function useTripPlanner() { if (n) { const existing = places.find(p => p.name?.trim().toLowerCase() === n) ?? places.find(p => p.name && (p.name.toLowerCase().includes(n) || n.includes(p.name.toLowerCase()))) - if (existing) return existing.id + // Only a server-side id may be linked. A negative id is an offline temp id + // (mutationQueue.nextTempId): the reservation write is online-only, the queue + // rewrites temp ids in a URL but never inside another entity's body, and + // day_accommodations.place_id carries a foreign key — so a temp id here is a + // rolled-back insert and a 500 instead of a saved booking. + if (existing && existing.id > 0) return existing.id } + // Offline the booking itself cannot be written (reservations are online-only), + // so minting a place here would only leave an orphan behind on the next flush + // — and its temp id could never be linked anyway. Link nothing, and skip the + // geocode round-trip too; the retry online matches this venue by name. + if (isEffectivelyOffline()) return null let lat: number | null = null let lng: number | null = null let address: string | null = venue.address ?? null @@ -939,8 +949,8 @@ export function useTripPlanner() { // learned about the previous one and the name match above could not // find it. addPlace unwraps the response and puts the place into // `places`, so the next save reuses it. - const place = await tripActions.addPlace(tripId, { name: name || address || 'Accommodation', lat, lng, address } as never) - return place?.id ?? null + const place = await tripActions.addPlace(tripId, { name: name || address || 'Accommodation', lat, lng, address }) + return place && place.id > 0 ? place.id : null } catch { return null } }