From aacc7f2c8d944ba52e813daf6021eb10acfd00eb Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 20 Oct 2026 14:02:20 +0200 Subject: [PATCH] fix(tours): open a tour's packing list on Packing, and ask before letting go of it while Packing is off "Open packing list" switched to the Lists tab but left it on whichever sub-tab was open last, so someone who had been in the to-dos landed there. It now picks the packing side first, and a Lists tab already on screen follows the choice; storage that is blocked no longer throws either. With the Packing addon off, "Remove link" let go of the link on one click, and the link cannot be made again until Packing is back on. It now opens the tour's packing dialog with only the dimmed link and the Remove link and Cancel buttons, so nothing changes without a second, deliberate click. --- .../Tours/packing/TourPackingLinkDialog.tsx | 111 ++++++++++-------- .../Tours/packing/TourPackingSection.tsx | 35 ++---- .../Tours/packing/packingLinkParts.tsx | 22 +++- .../Tours/packing/tourPacking.test.tsx | 21 +++- client/src/hooks/useListsSubTab.test.ts | 41 ++++++- client/src/hooks/useListsSubTab.ts | 44 ++++++- .../tripPlanner/useTourDetailActions.test.ts | 32 +++-- .../pages/tripPlanner/useTourDetailActions.ts | 4 +- 8 files changed, 218 insertions(+), 92 deletions(-) diff --git a/client/src/components/Tours/packing/TourPackingLinkDialog.tsx b/client/src/components/Tours/packing/TourPackingLinkDialog.tsx index 8f42aaaa5..e44ee95db 100644 --- a/client/src/components/Tours/packing/TourPackingLinkDialog.tsx +++ b/client/src/components/Tours/packing/TourPackingLinkDialog.tsx @@ -14,7 +14,7 @@ import { } from '../../shared/DialogShell'; import { Tooltip } from '../../shared/Tooltip'; import { ChoiceList, ChoiceRow } from '../collection/choiceParts'; -import { CategoryDot, CountBadge } from './packingLinkParts'; +import { CategoryDot, CountBadge, DimmedLink } from './packingLinkParts'; import type { TourPackingLinkController } from './useTourPackingLink'; /** One thing to point at: its mark, its name and how many items it holds. */ @@ -44,16 +44,57 @@ function Choice({ ); } +/** Everything the tour can point at: the whole list first, then each category. */ +function Choices({ packing }: { packing: TourPackingLinkController }) { + const { t } = useTranslation(); + return ( + + + packing.choose(null)} + testId="tour-packing-choice-whole" + mark={ + + + + } + name={t('tours.packing.whole')} + count={packing.wholeCount} + /> + {packing.categories.map((category) => ( + packing.choose(category.name)} + testId={`tour-packing-choice-${category.name}`} + mark={ + + + + } + name={category.name} + count={category.count} + /> + ))} + + + ); +} + /** * P2D-XD-01: the dialog that points a tour at its trip's packing list, as a * whole or at one of its categories, each with its colour and how many items * it holds. It writes only through the button in its foot; letting go of a - * link is offered there too, and neither touches the list itself. + * link is offered there too, and neither touches the list itself. With Packing + * off there is nothing to choose and a link cannot be made again, so it shows + * only the dimmed link and asks before letting go of it. */ export function TourPackingLinkDialog({ packing, tourName }: { packing: TourPackingLinkController; tourName: string }) { const { t } = useTranslation(); const labelId = useId(); if (!packing.open) return null; + const linkOnly = !packing.packingEnabled; return ( {t('common.cancel')} - void packing.confirm()} - icon={ - packing.saving ? ( - - ) : ( - - ) - } - > - {t('tours.packing.confirm')} - + {!linkOnly && ( + void packing.confirm()} + icon={ + packing.saving ? ( + + ) : ( + + ) + } + > + {t('tours.packing.confirm')} + + )} } > - - - packing.choose(null)} - testId="tour-packing-choice-whole" - mark={ - - - - } - name={t('tours.packing.whole')} - count={packing.wholeCount} - /> - {packing.categories.map((category) => ( - packing.choose(category.name)} - testId={`tour-packing-choice-${category.name}`} - mark={ - - - - } - name={category.name} - count={category.count} - /> - ))} - - + {linkOnly && packing.link ? : } ); } diff --git a/client/src/components/Tours/packing/TourPackingSection.tsx b/client/src/components/Tours/packing/TourPackingSection.tsx index 51c5b2142..841c7be16 100644 --- a/client/src/components/Tours/packing/TourPackingSection.tsx +++ b/client/src/components/Tours/packing/TourPackingSection.tsx @@ -1,14 +1,13 @@ -import type { TourListItem, TourPackingLink } from '@trek/shared'; -import { AlertTriangle, ArrowUpRight, Backpack, Check, Link2, PackageOpen, PowerOff, Unlink } from 'lucide-react'; +import type { TourListItem } from '@trek/shared'; +import { AlertTriangle, ArrowUpRight, Backpack, Check, Link2, PackageOpen, Unlink } from 'lucide-react'; import type { ReactNode } from 'react'; import { useTranslation } from '../../../i18n'; import type { PackingItem } from '../../../types'; import { BOX } from '../../Planner/bookings/bookingParts'; import { SoftPill, WhiteBadge } from '../../Planner/planParts'; import { DialogSection, fs } from '../../shared/DialogShell'; -import { Tooltip } from '../../shared/Tooltip'; import { CardAction, TourNotice } from '../tourParts'; -import { CategoryDot } from './packingLinkParts'; +import { CategoryDot, DimmedLink, linkName } from './packingLinkParts'; import { TourPackingLinkDialog } from './TourPackingLinkDialog'; import { useTourPackingLink, type TourPackingLinkController } from './useTourPackingLink'; @@ -71,11 +70,6 @@ function ItemRow({ item }: { item: PackingItem }) { ); } -/** The name of what the tour points at: a category by its own name, otherwise the whole list. */ -function linkName(link: TourPackingLink, t: (key: string) => string): string { - return link.category ?? t('tours.packing.whole'); -} - /** The linked list as it reads now: its items with how many are packed, or why there are none to show. */ function LinkedCard({ packing, onOpenPacking }: { packing: TourPackingLinkController; onOpenPacking?: () => void }) { const { t } = useTranslation(); @@ -192,17 +186,7 @@ function SectionBody({ ); } if (!packing.link) return ; - if (!packing.packingEnabled) { - // Dimmed, not hidden: the tour still points at the list, it just cannot be shown. - return ( -
- }> - {linkName(packing.link, t)} - {t('tours.packing.addonOff')} - -
- ); - } + if (!packing.packingEnabled) return ; return ; } @@ -239,14 +223,11 @@ export function TourPackingSection({ ); } else if (editable && packing.link) { + // With Packing off a link cannot be made again, so letting go asks first, in the tour's packing dialog. action = ( - - - void packing.unlink()} icon={}> - {t('tours.packing.unlink')} - - - + }> + {t('tours.packing.unlink')} + ); } return ( diff --git a/client/src/components/Tours/packing/packingLinkParts.tsx b/client/src/components/Tours/packing/packingLinkParts.tsx index 9ad51e9cb..c040df278 100644 --- a/client/src/components/Tours/packing/packingLinkParts.tsx +++ b/client/src/components/Tours/packing/packingLinkParts.tsx @@ -1,6 +1,26 @@ -import { Package } from 'lucide-react'; +import type { TourPackingLink } from '@trek/shared'; +import { Package, PowerOff } from 'lucide-react'; import { useTranslation } from '../../../i18n'; import { tintOf, WhiteBadge } from '../../Planner/planParts'; +import { TourNotice } from '../tourParts'; + +/** The name of what the tour points at: a category by its own name, otherwise the whole list. */ +export function linkName(link: TourPackingLink, t: (key: string) => string): string { + return link.category ?? t('tours.packing.whole'); +} + +/** A link while Packing is off: dimmed, not hidden, since the tour still points at the list it cannot show. */ +export function DimmedLink({ link }: { link: TourPackingLink }) { + const { t } = useTranslation(); + return ( +
+ }> + {linkName(link, t)} + {t('tours.packing.addonOff')} + +
+ ); +} /** A packing category's own colour, as the packing list gives it, with a soft halo of the same colour. */ export function CategoryDot({ color }: { color: string }) { diff --git a/client/src/components/Tours/packing/tourPacking.test.tsx b/client/src/components/Tours/packing/tourPacking.test.tsx index afc5dcb17..d5cf95455 100644 --- a/client/src/components/Tours/packing/tourPacking.test.tsx +++ b/client/src/components/Tours/packing/tourPacking.test.tsx @@ -12,7 +12,7 @@ import type { PackingItem } from '../../../types'; import { TourPackingSection } from './TourPackingSection'; import { useTourPackingLink } from './useTourPackingLink'; -// FE-TOUR-PACKING-001 to FE-TOUR-PACKING-010 +// FE-TOUR-PACKING-001 to FE-TOUR-PACKING-011 const toast = vi.hoisted(() => ({ success: vi.fn(), error: vi.fn() })); vi.mock('../../shared/Toast', () => ({ useToast: () => toast })); @@ -201,10 +201,29 @@ describe('TourPackingSection', () => { section(); expect(await screen.findByText('Packing lists are switched off, so the list cannot be shown.')).toBeInTheDocument(); fireEvent.click(screen.getByText('Remove link')); + // One click only asks: the dialog shows the dimmed link, nothing to choose and no way to link again. + expect(unlink).not.toHaveBeenCalled(); + const dialog = await screen.findByRole('dialog'); + expect(within(dialog).getByTestId('tour-packing-dimmed')).toBeInTheDocument(); + expect(within(dialog).queryByTestId('tour-packing-choice-whole')).not.toBeInTheDocument(); + expect(within(dialog).queryByTestId('tour-packing-confirm')).not.toBeInTheDocument(); + fireEvent.click(within(dialog).getByTestId('tour-packing-unlink')); await waitFor(() => expect(unlink).toHaveBeenCalledWith(5, 11)); await waitFor(() => expect(screen.queryByTestId('tour-packing-section')).not.toBeInTheDocument()); }); + it('FE-TOUR-PACKING-011: with Packing off, cancelling the question keeps the link', async () => { + setPacking(false); + vi.mocked(tourPackingLinkRepo.read).mockResolvedValue(GEAR); + const unlink = vi.spyOn(tourPackingLinkRepo, 'unlink').mockResolvedValue(null); + section(); + fireEvent.click(await screen.findByText('Remove link')); + fireEvent.click(within(await screen.findByRole('dialog')).getByText('Cancel')); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + expect(unlink).not.toHaveBeenCalled(); + expect(screen.getByTestId('tour-packing-dimmed')).toBeInTheDocument(); + }); + it('FE-TOUR-PACKING-010: a failed read is said, with a retry', async () => { vi.mocked(tourPackingLinkRepo.read).mockRejectedValueOnce(new Error('500')); section(); diff --git a/client/src/hooks/useListsSubTab.test.ts b/client/src/hooks/useListsSubTab.test.ts index b46917549..54dbc9505 100644 --- a/client/src/hooks/useListsSubTab.test.ts +++ b/client/src/hooks/useListsSubTab.test.ts @@ -1,7 +1,7 @@ import { act, renderHook } from '@testing-library/react'; -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; -import { useListsSubTab } from './useListsSubTab'; +import { rememberListsSubTab, useListsSubTab } from './useListsSubTab'; // FE-HOOK-LISTSTAB-001 to FE-HOOK-LISTSTAB-006 @@ -47,3 +47,40 @@ describe('useListsSubTab', () => { expect(renderHook(() => useListsSubTab(3, { onlyKnownTabs: true })).result.current[0]).toBe('todo'); }); }); + +// FE-HOOK-LISTSTAB-007 to FE-HOOK-LISTSTAB-009: picking the sub-tab from outside the Lists tab. +describe('rememberListsSubTab', () => { + it('FE-HOOK-LISTSTAB-007: the Lists tab opens on the sub-tab picked before it was on screen', () => { + sessionStorage.setItem('trip-lists-subtab-7', 'todo'); + rememberListsSubTab(7, 'packing'); + expect(renderHook(() => useListsSubTab(7)).result.current[0]).toBe('packing'); + }); + + it('FE-HOOK-LISTSTAB-008: a Lists tab already on screen follows, one for another trip does not', () => { + sessionStorage.setItem('trip-lists-subtab-7', 'todo'); + sessionStorage.setItem('trip-lists-subtab-8', 'todo'); + const same = renderHook(() => useListsSubTab(7)); + const other = renderHook(() => useListsSubTab(8)); + act(() => rememberListsSubTab(7, 'packing')); + expect(same.result.current[0]).toBe('packing'); + expect(other.result.current[0]).toBe('todo'); + same.unmount(); + act(() => rememberListsSubTab(7, 'todo')); + expect(sessionStorage.getItem('trip-lists-subtab-7')).toBe('todo'); + }); + + it('FE-HOOK-LISTSTAB-009: blocked storage still switches the tab on screen and does not throw', () => { + const { result } = renderHook(() => useListsSubTab(7)); + const setItem = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { + throw new Error('blocked'); + }); + const getItem = vi.spyOn(Storage.prototype, 'getItem').mockImplementation(() => { + throw new Error('blocked'); + }); + act(() => rememberListsSubTab(7, 'todo')); + expect(result.current[0]).toBe('todo'); + expect(renderHook(() => useListsSubTab(9)).result.current[0]).toBe('packing'); + setItem.mockRestore(); + getItem.mockRestore(); + }); +}); diff --git a/client/src/hooks/useListsSubTab.ts b/client/src/hooks/useListsSubTab.ts index f75e7efa4..481b72b91 100644 --- a/client/src/hooks/useListsSubTab.ts +++ b/client/src/hooks/useListsSubTab.ts @@ -1,10 +1,39 @@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; /** The two lists the Lists tab switches between. */ export type ListsSubTab = 'packing' | 'todo'; const storageKey = (tripId: number) => `trip-lists-subtab-${tripId}`; +/** Lists tabs that are on screen, told when another surface picks their sub-tab. */ +const followers = new Set<(tripId: number, tab: ListsSubTab) => void>(); + +function readStored(tripId: number): string | null { + try { + return sessionStorage.getItem(storageKey(tripId)); + } catch { + return null; + } +} + +function store(tripId: number, tab: ListsSubTab) { + try { + sessionStorage.setItem(storageKey(tripId), tab); + } catch { + // Storage can be blocked; the choice then lasts as long as the tab is on screen. + } +} + +/** + * Picks the Lists tab's sub-tab from outside it, e.g. "Open packing list" on a + * tour. The Lists tab opens with it next time, and one already on screen + * switches to it right away. + */ +export function rememberListsSubTab(tripId: number, tab: ListsSubTab) { + store(tripId, tab); + followers.forEach((follow) => follow(tripId, tab)); +} + /** * The Lists tab's sub-tab, remembered per trip for the browser session. Both the * desktop container and the phone shell read and write the same key. The phone @@ -13,13 +42,22 @@ const storageKey = (tripId: number) => `trip-lists-subtab-${tripId}`; */ export function useListsSubTab(tripId: number, { onlyKnownTabs = false }: { onlyKnownTabs?: boolean } = {}) { const [subTab, setSubTabState] = useState(() => { - const saved = sessionStorage.getItem(storageKey(tripId)); + const saved = readStored(tripId); if (onlyKnownTabs) return saved === 'todo' ? 'todo' : 'packing'; return (saved as ListsSubTab) || 'packing'; }); + useEffect(() => { + const follow = (forTrip: number, tab: ListsSubTab) => { + if (forTrip === tripId) setSubTabState(tab); + }; + followers.add(follow); + return () => { + followers.delete(follow); + }; + }, [tripId]); const setSubTab = (tab: ListsSubTab) => { setSubTabState(tab); - sessionStorage.setItem(storageKey(tripId), tab); + store(tripId, tab); }; return [subTab, setSubTab] as const; } diff --git a/client/src/pages/tripPlanner/useTourDetailActions.test.ts b/client/src/pages/tripPlanner/useTourDetailActions.test.ts index c2cd05044..c409ba9be 100644 --- a/client/src/pages/tripPlanner/useTourDetailActions.test.ts +++ b/client/src/pages/tripPlanner/useTourDetailActions.test.ts @@ -1,17 +1,33 @@ // P2D-XD-01: from a tour's packing link to the trip's Lists tab, the tour's detail closed on the way. -import { renderHook } from '@testing-library/react'; +import { act, renderHook } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; +import { useListsSubTab } from '../../hooks/useListsSubTab'; import { useTourDetailActions } from './useTourDetailActions'; import type { useTripPlanner } from './useTripPlanner'; describe('useTourDetailActions', () => { + const planner = () => ({ tripId: 5, setSelectedPlaceId: vi.fn(), handleTabChange: vi.fn() }); + const actions = (p: ReturnType) => + renderHook(() => useTourDetailActions(p as unknown as ReturnType)).result.current; + it('FE-TOUR-DETAIL-ACTIONS-001: opening the packing list closes the tour and switches to the Lists tab', () => { - const planner = { setSelectedPlaceId: vi.fn(), handleTabChange: vi.fn() }; - const { result } = renderHook(() => - useTourDetailActions(planner as unknown as ReturnType) - ); - result.current.onOpenPacking(); - expect(planner.setSelectedPlaceId).toHaveBeenCalledWith(null); - expect(planner.handleTabChange).toHaveBeenCalledWith('listen'); + const p = planner(); + actions(p).onOpenPacking(); + expect(p.setSelectedPlaceId).toHaveBeenCalledWith(null); + expect(p.handleTabChange).toHaveBeenCalledWith('listen'); + }); + + it('FE-TOUR-DETAIL-ACTIONS-002: the Lists tab opens on packing even when the to-dos were last open', () => { + sessionStorage.setItem('trip-lists-subtab-5', 'todo'); + actions(planner()).onOpenPacking(); + expect(renderHook(() => useListsSubTab(5)).result.current[0]).toBe('packing'); + }); + + it('FE-TOUR-DETAIL-ACTIONS-003: a Lists tab already on screen on the to-dos switches to packing', () => { + sessionStorage.setItem('trip-lists-subtab-5', 'todo'); + const lists = renderHook(() => useListsSubTab(5)); + expect(lists.result.current[0]).toBe('todo'); + act(() => actions(planner()).onOpenPacking()); + expect(lists.result.current[0]).toBe('packing'); }); }); diff --git a/client/src/pages/tripPlanner/useTourDetailActions.ts b/client/src/pages/tripPlanner/useTourDetailActions.ts index 62d0a21d6..6b87d76e9 100644 --- a/client/src/pages/tripPlanner/useTourDetailActions.ts +++ b/client/src/pages/tripPlanner/useTourDetailActions.ts @@ -1,4 +1,5 @@ import type { TourListItem } from '@trek/shared'; +import { rememberListsSubTab } from '../../hooks/useListsSubTab'; import { tourRepo } from '../../repo/tourRepo'; import { useTripStore } from '../../store/tripStore'; import type { Place } from '../../types'; @@ -36,9 +37,10 @@ export function useTourDetailActions(planner: ReturnType) onToursChanged: (placeIds: number[]) => invalidateTourPlaceIds({ placeIds }), /** LIB-02: a tour taken over from another trip; the tours rail opens it in the planner. */ onTakenOver: bringIn, - /** P2D-XD-01: the packing list a tour points at, in the trip's Lists tab. */ + /** P2D-XD-01: the packing list a tour points at, in the trip's Lists tab, on its packing side. */ onOpenPacking: () => { planner.setSelectedPlaceId(null); + rememberListsSubTab(tripId, 'packing'); planner.handleTabChange('listen'); }, onSaveAsNew: async (tour: TourListItem) => {