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.
2026-10-20 22:16:26 +00:00 · 2026-10-20 14:02:20 +02:00
parent f833a05461
commit aacc7f2c8d
8 changed files with 218 additions and 92 deletions
@@ -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 (
+    <DialogSection label={t('tours.packing.choose')}>
+      <ChoiceList label={t('tours.packing.choose')} tall>
+        <Choice
+          selected={packing.choice === null}
+          onChoose={() => packing.choose(null)}
+          testId="tour-packing-choice-whole"
+          mark={
+            <span className="grid h-6 w-6 flex-none place-items-center rounded-full bg-surface-card text-content-muted shadow-sm">
+              <ListChecks size={13} strokeWidth={2.2} />
+            </span>
+          }
+          name={t('tours.packing.whole')}
+          count={packing.wholeCount}
+        />
+        {packing.categories.map((category) => (
+          <Choice
+            key={category.name}
+            selected={packing.choice === category.name}
+            onChoose={() => packing.choose(category.name)}
+            testId={`tour-packing-choice-${category.name}`}
+            mark={
+              <span className="grid h-6 w-6 flex-none place-items-center">
+                <CategoryDot color={category.color} />
+              </span>
+            }
+            name={category.name}
+            count={category.count}
+          />
+        ))}
+      </ChoiceList>
+    </DialogSection>
+  );
+}
+
 /**
 * 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 (
    <DialogShell
      onClose={packing.close}
@@ -71,7 +112,7 @@ export function TourPackingLinkDialog({ packing, tourName }: { packing: TourPack
          tint={NEUTRAL_TINT}
          labelId={labelId}
          onClose={packing.close}
-          eyebrow={t('tours.packing.link')}
+          eyebrow={linkOnly ? t('tours.packing.unlink') : t('tours.packing.link')}
          title={tourName}
          sub={t('tours.packing.dialogSub')}
          subWraps
@@ -97,55 +138,27 @@ export function TourPackingLinkDialog({ packing, tourName }: { packing: TourPack
          <DialogButton onClick={packing.close} disabled={packing.saving}>
            {t('common.cancel')}
          </DialogButton>
-          <DialogButton
-            variant="primary"
-            data-testid="tour-packing-confirm"
-            disabled={!packing.canConfirm}
-            onClick={() => void packing.confirm()}
-            icon={
-              packing.saving ? (
-                <Loader2 size={14} strokeWidth={2} className="animate-spin" />
-              ) : (
-                <Link2 size={14} strokeWidth={2} />
-              )
-            }
-          >
-            {t('tours.packing.confirm')}
-          </DialogButton>
+          {!linkOnly && (
+            <DialogButton
+              variant="primary"
+              data-testid="tour-packing-confirm"
+              disabled={!packing.canConfirm}
+              onClick={() => void packing.confirm()}
+              icon={
+                packing.saving ? (
+                  <Loader2 size={14} strokeWidth={2} className="animate-spin" />
+                ) : (
+                  <Link2 size={14} strokeWidth={2} />
+                )
+              }
+            >
+              {t('tours.packing.confirm')}
+            </DialogButton>
+          )}
        </DialogFooter>
      }
    >
-      <DialogSection label={t('tours.packing.choose')}>
-        <ChoiceList label={t('tours.packing.choose')} tall>
-          <Choice
-            selected={packing.choice === null}
-            onChoose={() => packing.choose(null)}
-            testId="tour-packing-choice-whole"
-            mark={
-              <span className="grid h-6 w-6 flex-none place-items-center rounded-full bg-surface-card text-content-muted shadow-sm">
-                <ListChecks size={13} strokeWidth={2.2} />
-              </span>
-            }
-            name={t('tours.packing.whole')}
-            count={packing.wholeCount}
-          />
-          {packing.categories.map((category) => (
-            <Choice
-              key={category.name}
-              selected={packing.choice === category.name}
-              onChoose={() => packing.choose(category.name)}
-              testId={`tour-packing-choice-${category.name}`}
-              mark={
-                <span className="grid h-6 w-6 flex-none place-items-center">
-                  <CategoryDot color={category.color} />
-                </span>
-              }
-              name={category.name}
-              count={category.count}
-            />
-          ))}
-        </ChoiceList>
-      </DialogSection>
+      {linkOnly && packing.link ? <DimmedLink link={packing.link} /> : <Choices packing={packing} />}
    </DialogShell>
  );
 }
@@ -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 <UnlinkedCard onLink={canEdit ? packing.show : undefined} />;
-  if (!packing.packingEnabled) {
-    // Dimmed, not hidden: the tour still points at the list, it just cannot be shown.
-    return (
-      <div className="opacity-70">
-        <TourNotice icon={<PowerOff size={13} strokeWidth={2} />}>
-          <span className="font-semibold text-content-secondary">{linkName(packing.link, t)}</span>
-          <span className="block">{t('tours.packing.addonOff')}</span>
-        </TourNotice>
-      </div>
-    );
-  }
+  if (!packing.packingEnabled) return <DimmedLink link={packing.link} />;
  return <LinkedCard packing={packing} onOpenPacking={onOpenPacking} />;
 }

@@ -239,14 +223,11 @@ export function TourPackingSection({
      </SectionPill>
    );
  } 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 = (
-      <Tooltip label={t('tours.packing.unlinkHint')}>
-        <span className="inline-flex">
-          <SectionPill onClick={() => void packing.unlink()} icon={<Unlink size={11} strokeWidth={2} />}>
-            {t('tours.packing.unlink')}
-          </SectionPill>
-        </span>
-      </Tooltip>
+      <SectionPill onClick={packing.show} icon={<Unlink size={11} strokeWidth={2} />}>
+        {t('tours.packing.unlink')}
+      </SectionPill>
    );
  }
  return (
@@ -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 (
+    <div className="opacity-70" data-testid="tour-packing-dimmed">
+      <TourNotice icon={<PowerOff size={13} strokeWidth={2} />}>
+        <span className="font-semibold text-content-secondary">{linkName(link, t)}</span>
+        <span className="block">{t('tours.packing.addonOff')}</span>
+      </TourNotice>
+    </div>
+  );
+}

 /** 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 }) {
@@ -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();
@@ -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();
+  });
+});
@@ -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<ListsSubTab>(() => {
-    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;
 }
@@ -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<typeof planner>) =>
+    renderHook(() => useTourDetailActions(p as unknown as ReturnType<typeof useTripPlanner>)).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<typeof useTripPlanner>)
-    );
-    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');
  });
 });
@@ -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<typeof useTripPlanner>)
    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) => {