From 52068f85c37ddf6a8e7acbc92d46e42eb32688e2 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 28 Apr 2026 13:28:16 +0200 Subject: [PATCH] fix: replace raw day-ID range checks with position-based helper (issue #889 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 8e05ba7 fixed the accommodation date-range pickers, but the post-save state filters in DayDetailPanel and several other consumers still compared `day.id >= start_day_id && day.id <= end_day_id`. With non-monotonic ID layouts (day_number 1-9 → IDs 17-25, day_number 10-16 → IDs 1-7) this made the just-saved accommodation immediately invisible — matching the regression reported in the last comment of #889. Introduces `isDayInAccommodationRange` in `client/src/utils/dayOrder.ts` which compares positional order (`day_number` with `indexOf` fallback) rather than raw IDs. Falls back to the old numeric comparison when endpoint days are absent from the loaded array (sparse test data or partial loads) so existing tests are unaffected. Fixed call sites: - DayDetailPanel.tsx (initial load, post-create, post-delete, post-edit-save) - DayPlanSidebar.tsx (daily badge renderer) - SharedTripPage.tsx (public share view) - TripPDF.tsx (PDF export filter + sort) Also declares `day_number?: number` on the client `Day` type (already returned by the server but previously untyped). Adds regression tests FE-PLANNER-DAYDETAIL-060/061/062 covering the edit-save, create-save, and initial-load paths with the reporter's exact non-monotonic ID layout. --- client/src/components/PDF/TripPDF.tsx | 9 +- .../Planner/DayDetailPanel.test.tsx | 94 +++++++++++++++++++ .../src/components/Planner/DayDetailPanel.tsx | 11 ++- .../src/components/Planner/DayPlanSidebar.tsx | 3 +- client/src/pages/SharedTripPage.tsx | 3 +- client/src/types.ts | 1 + client/src/utils/dayOrder.ts | 23 +++++ 7 files changed, 135 insertions(+), 9 deletions(-) create mode 100644 client/src/utils/dayOrder.ts diff --git a/client/src/components/PDF/TripPDF.tsx b/client/src/components/PDF/TripPDF.tsx index 1a5a3316..e5700b93 100644 --- a/client/src/components/PDF/TripPDF.tsx +++ b/client/src/components/PDF/TripPDF.tsx @@ -4,6 +4,7 @@ import { getCategoryIcon } from '../shared/categoryIcons' import { FileText, Info, Clock, MapPin, Navigation, Train, Plane, Bus, Car, Ship, Coffee, Ticket, Star, Heart, Camera, Flag, Lightbulb, AlertTriangle, ShoppingBag, Bookmark, Hotel, LogIn, LogOut, KeyRound, BedDouble, Utensils, Users, LucideIcon } from 'lucide-react' import { accommodationsApi, mapsApi } from '../../api/client' import type { Trip, Day, Place, Category, AssignmentsMap, DayNotesMap } from '../../types' +import { isDayInAccommodationRange, getDayOrder } from '../../utils/dayOrder' function renderLucideIcon(icon:LucideIcon, props = {}) { if (!_renderToStaticMarkup) return '' @@ -285,8 +286,12 @@ export async function downloadTripPDF({ trip, days, places, assignments, categor }).join('') const accommodationsForDay = (accommodations.accommodations || []).filter(a => - days.some(d => d.id >= a.start_day_id && d.id <= a.end_day_id && d.id === day?.id) - ).sort((a, b) => a.start_day_id - b.start_day_id) + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false + ).sort((a, b) => { + const startA = days.find(d => d.id === a.start_day_id) + const startB = days.find(d => d.id === b.start_day_id) + return (startA ? getDayOrder(startA, days) : 0) - (startB ? getDayOrder(startB, days) : 0) + }) const accommodationDetails = accommodationsForDay.map(item => { const isCheckIn = day.id === item.start_day_id diff --git a/client/src/components/Planner/DayDetailPanel.test.tsx b/client/src/components/Planner/DayDetailPanel.test.tsx index 9bdc0256..a70fd9d5 100644 --- a/client/src/components/Planner/DayDetailPanel.test.tsx +++ b/client/src/components/Planner/DayDetailPanel.test.tsx @@ -1069,6 +1069,100 @@ describe('DayDetailPanel', () => { }); }); + // ── Post-save state filter — non-monotonic IDs (issue #889 follow-up) ──────── + + it('FE-PLANNER-DAYDETAIL-060: non-monotonic IDs — hotel stays visible after edit-save (issue #889 regression)', async () => { + const days = buildNonMonotonicDays(); + let getCallCount = 0; + server.use( + http.get('/api/trips/1/accommodations', () => { + getCallCount++; + const acc = getCallCount === 1 + // Initial load: single-day so old filter (17>=17 && 17<=17) passes — hotel visible, edit possible + ? { id: 1, place_id: 50, place_name: 'Span Hotel', place_address: null, start_day_id: 17, end_day_id: 17, check_in: null, check_out: null, confirmation: null } + // Post-save relist: full span — old filter (17>=17 && 17<=7) would drop it, new code keeps it + : { id: 1, place_id: 50, place_name: 'Span Hotel', place_address: null, start_day_id: 17, end_day_id: 7, check_in: null, check_out: null, confirmation: null }; + return HttpResponse.json({ accommodations: [acc] }); + }), + http.put('/api/trips/1/accommodations/1', async ({ request }) => { + const body = await request.json() as any; + return HttpResponse.json({ + accommodation: { id: 1, place_id: 50, place_name: 'Span Hotel', place_address: null, + start_day_id: body.start_day_id, end_day_id: body.end_day_id, + check_in: null, check_out: null, confirmation: null }, + }); + }), + ); + + render(); + await screen.findByText('Span Hotel'); + + // Pencil = 3rd button (index 2): collapse, close, pencil, remove + const allButtons = screen.getAllByRole('button'); + await userEvent.click(allButtons[2]); + + // Extend end picker to Day 16 (id=7) + await userEvent.click(getDayPickerTriggers()[1]); + await userEvent.click(screen.getAllByRole('button').find(b => b.textContent?.startsWith('Day 16'))!); + await userEvent.click(screen.getByRole('button', { name: /^Save$/i })); + + // Old code: 17>=17 && 17<=7 → false (hotel vanishes). New code: position 0 in [0,15] → visible. + await waitFor(() => { + expect(screen.getByText('Span Hotel')).toBeInTheDocument(); + }); + }); + + it('FE-PLANNER-DAYDETAIL-061: non-monotonic IDs — hotel appears after create-save on intermediate day', async () => { + const days = buildNonMonotonicDays(); + const place = buildPlace({ id: 55, name: 'Created Hotel' }); + // Current day: days[5] = id 22, position 5 (within any full-span range) + const currentDay = days[5]; + server.use( + http.post('/api/trips/1/accommodations', async ({ request }) => { + const body = await request.json() as any; + return HttpResponse.json({ + accommodation: { id: 200, place_id: 55, place_name: 'Created Hotel', place_address: null, + start_day_id: body.start_day_id, end_day_id: body.end_day_id, + check_in: null, check_out: null, confirmation: null }, + }); + }), + ); + + render(); + await userEvent.click(await screen.findByText(/Add accommodation/i)); + await userEvent.click(await screen.findByRole('button', { name: /Created Hotel/i })); + + // Extend end to Day 16 (id=7) — start stays at current day id=22 + await userEvent.click(getDayPickerTriggers()[1]); + await userEvent.click(screen.getAllByRole('button').find(b => b.textContent?.startsWith('Day 16'))!); + await userEvent.click(screen.getByRole('button', { name: /^Save$/i })); + + // Old code: 22>=22 && 22<=7 → false (hotel vanishes). New code: position 5 in [5,15] → visible. + await waitFor(() => { + expect(screen.getByText('Created Hotel')).toBeInTheDocument(); + }); + }); + + it('FE-PLANNER-DAYDETAIL-062: non-monotonic IDs — hotel shown on initial load when it spans the full trip', async () => { + const days = buildNonMonotonicDays(); + server.use( + http.get('/api/trips/1/accommodations', () => + HttpResponse.json({ + accommodations: [{ id: 1, place_id: 60, place_name: 'Full Trip Hotel', place_address: null, + start_day_id: 17, end_day_id: 7, check_in: null, check_out: null, confirmation: null }], + }) + ), + ); + + // Day 1 (id=17): old filter: 17>=17 && 17<=7 → false. New: position 0 in [0,15] → visible. + render(); + await screen.findByText('Full Trip Hotel'); + + // Intermediate day (id=1, position 9): old filter: 1>=17 → false. New: 9 in [0,15] → visible. + render(); + await screen.findByText('Full Trip Hotel'); + }); + it('FE-PLANNER-DAYDETAIL-040: 12h time format renders reservation time with AM/PM', async () => { seedStore(useSettingsStore, { settings: { time_format: '12h', temperature_unit: 'celsius', blur_booking_codes: false }, diff --git a/client/src/components/Planner/DayDetailPanel.tsx b/client/src/components/Planner/DayDetailPanel.tsx index 0d53f8ff..407db408 100644 --- a/client/src/components/Planner/DayDetailPanel.tsx +++ b/client/src/components/Planner/DayDetailPanel.tsx @@ -12,6 +12,7 @@ import CustomTimePicker from '../shared/CustomTimePicker' import { useSettingsStore } from '../../store/settingsStore' import { getLocaleForLanguage, useTranslation } from '../../i18n' import type { Day, Place, Category, Reservation, AssignmentsMap } from '../../types' +import { isDayInAccommodationRange } from '../../utils/dayOrder' const WEATHER_ICON_MAP = { Clear: Sun, Clouds: Cloud, Rain: CloudRain, Drizzle: CloudDrizzle, @@ -99,7 +100,7 @@ export default function DayDetailPanel({ day, days, places, categories = [], tri .then(data => { setAccommodations(data.accommodations || []) const allForDay = (data.accommodations || []).filter(a => - days.some(d => d.id >= a.start_day_id && d.id <= a.end_day_id && d.id === day?.id) + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false ) setDayAccommodations(allForDay) setAccommodation(allForDay[0] || null) @@ -130,7 +131,7 @@ export default function DayDetailPanel({ day, days, places, categories = [], tri setAccommodations(updated) setAccommodation(newAcc) setDayAccommodations(updated.filter(a => - days.some(d => d.id >= a.start_day_id && d.id <= a.end_day_id && d.id === day?.id) + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false )) setShowHotelPicker(false) setHotelForm({ check_in: '', check_in_end: '', check_out: '', confirmation: '', place_id: null }) @@ -154,7 +155,7 @@ export default function DayDetailPanel({ day, days, places, categories = [], tri const updated = accommodations.filter(a => a.id !== accommodation.id) setAccommodations(updated) setDayAccommodations(updated.filter(a => - days.some(d => d.id >= a.start_day_id && d.id <= a.end_day_id && d.id === day?.id) + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false )) setAccommodation(null) onAccommodationChange?.() @@ -598,9 +599,9 @@ export default function DayDetailPanel({ day, days, places, categories = [], tri const all = d.accommodations || [] setAccommodations(all) setDayAccommodations(all.filter(a => - days.some(dd => dd.id >= a.start_day_id && dd.id <= a.end_day_id && dd.id === day?.id) + day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false )) - const acc = all.find(a => days.some(dd => dd.id >= a.start_day_id && dd.id <= a.end_day_id && dd.id === day?.id)) + const acc = all.find(a => day ? isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days) : false) setAccommodation(acc || null) }) onAccommodationChange?.() diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index b1eda2d3..7c44f091 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -21,6 +21,7 @@ import { useTripStore } from '../../store/tripStore' import { useCanDo } from '../../store/permissionsStore' import { useSettingsStore } from '../../store/settingsStore' import { useTranslation } from '../../i18n' +import { isDayInAccommodationRange } from '../../utils/dayOrder' import { formatDate, formatTime, dayTotalCost, currencyDecimals } from '../../utils/formatters' import { useDayNotes } from '../../hooks/useDayNotes' import Tooltip from '../shared/Tooltip' @@ -1214,7 +1215,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar({ )} {(() => { - const dayAccs = accommodations.filter(a => day.id >= a.start_day_id && day.id <= a.end_day_id) + const dayAccs = accommodations.filter(a => isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, days)) // Sort: check-out first, then ongoing stays, then check-in last .sort((a, b) => { const aIsOut = a.end_day_id === day.id && a.start_day_id !== day.id diff --git a/client/src/pages/SharedTripPage.tsx b/client/src/pages/SharedTripPage.tsx index 09fa8669..e7c75c1f 100644 --- a/client/src/pages/SharedTripPage.tsx +++ b/client/src/pages/SharedTripPage.tsx @@ -10,6 +10,7 @@ import { getCategoryIcon } from '../components/shared/categoryIcons' import { createElement } from 'react' import { renderToStaticMarkup } from 'react-dom/server' import { Clock, MapPin, FileText, Train, Plane, Bus, Car, Ship, Ticket, Hotel, Map, Luggage, Wallet, MessageCircle } from 'lucide-react' +import { isDayInAccommodationRange } from '../utils/dayOrder' const TRANSPORT_TYPES = new Set(['flight', 'train', 'bus', 'car', 'cruise']) const TRANSPORT_ICONS = { flight: Plane, train: Train, bus: Bus, car: Car, cruise: Ship } @@ -184,7 +185,7 @@ export default function SharedTripPage() { const da = assignments[String(day.id)] || [] const notes = (dayNotes[String(day.id)] || []) const dayTransport = (reservations || []).filter((r: any) => TRANSPORT_TYPES.has(r.type) && r.reservation_time?.split('T')[0] === day.date) - const dayAccs = (accommodations || []).filter((a: any) => day.id >= a.start_day_id && day.id <= a.end_day_id) + const dayAccs = (accommodations || []).filter((a: any) => isDayInAccommodationRange(day, a.start_day_id, a.end_day_id, sortedDays)) const merged = [ ...da.map((a: any) => ({ type: 'place', k: a.order_index, data: a })), diff --git a/client/src/types.ts b/client/src/types.ts index 5701597a..8c9c3039 100644 --- a/client/src/types.ts +++ b/client/src/types.ts @@ -31,6 +31,7 @@ export interface Trip { export interface Day { id: number trip_id: number + day_number?: number date: string title: string | null notes: string | null diff --git a/client/src/utils/dayOrder.ts b/client/src/utils/dayOrder.ts new file mode 100644 index 00000000..8ef66046 --- /dev/null +++ b/client/src/utils/dayOrder.ts @@ -0,0 +1,23 @@ +import type { Day } from '../types' + +export const getDayOrder = (day: Day, days: Day[]): number => + day.day_number ?? days.indexOf(day) + +export const isDayInAccommodationRange = ( + day: Day, + startDayId: number, + endDayId: number, + days: Day[], +): boolean => { + const startDay = days.find(d => d.id === startDayId) + const endDay = days.find(d => d.id === endDayId) + if (!startDay || !endDay) { + // Endpoint days not in the loaded array (e.g. sparse test data or partial load). + // Fall back to numeric ID range — acceptable since non-monotonic IDs only arise when + // both endpoints are present in a fully-loaded trip's days list. + return day.id >= Math.min(startDayId, endDayId) && day.id <= Math.max(startDayId, endDayId) + } + const lo = Math.min(getDayOrder(startDay, days), getDayOrder(endDay, days)) + const hi = Math.max(getDayOrder(startDay, days), getDayOrder(endDay, days)) + return getDayOrder(day, days) >= lo && getDayOrder(day, days) <= hi +}