refactor(collections): share the bulk save to list logic between desktop and phone

useSaveTripPlacesToList holds the logic that saves a selection of trip
places to a list, which SaveTripPlacesToListModal and
MPlacesSaveToCollectionSheet each carried as a copy. The two copies did
the same thing, so the hook has no variants.

client/src/components/Collections/SaveTripPlacesToListModal.tsx

@@ -1,12 +1,9 @@
-import React, { useEffect, useId, useMemo, useState } from 'react'
+import React, { useId } from 'react'
 import { Search, Bookmark, ArrowRight, Loader2 } from 'lucide-react'
 import { DialogButton, DialogFooter, DialogHeader, DialogShell, DialogTile, FooterSpacer, NEUTRAL_TINT, fs } from '../shared/DialogShell'
 import { INPUT } from '../shared/dialogParts'
-import { useToast } from '../shared/Toast'
 import { useTranslation } from '../../i18n'
-import { collectionsApi } from '../../api/collections'
-import { getApiErrorMessage } from '../../utils/apiError'
-import type { Collection } from '@trek/shared'
+import { useSaveTripPlacesToList } from './useSaveTripPlacesToList'
 
 interface SaveTripPlacesToListModalProps {
   isOpen: boolean
@@ -24,51 +21,13 @@ interface SaveTripPlacesToListModalProps {
  */
 export default function SaveTripPlacesToListModal({ isOpen, tripId, placeIds, onClose, onDone }: SaveTripPlacesToListModalProps): React.ReactElement | null {
   const { t } = useTranslation()
-  const toast = useToast()
   const labelId = useId()
-  const [lists, setLists] = useState<Collection[]>([])
-  const [loading, setLoading] = useState(false)
-  const [search, setSearch] = useState('')
-  const [busyId, setBusyId] = useState<number | null>(null)
-
-  useEffect(() => {
-    if (!isOpen) return
-    let cancelled = false
-    setLoading(true)
-    setSearch('')
-    collectionsApi.list()
-      // Only lists the user can add to (their own or an editor/admin share). The
-      // server still enforces this; here we drop lists that are clearly read-only.
-      .then(res => { if (!cancelled) setLists((res.collections ?? []).filter(c => c.is_owner !== false)) })
-      .catch(() => { if (!cancelled) setLists([]) })
-      .finally(() => { if (!cancelled) setLoading(false) })
-    return () => { cancelled = true }
-  }, [isOpen])
-
-  const filtered = useMemo(() => {
-    const q = search.trim().toLowerCase()
-    return q ? lists.filter(l => l.name.toLowerCase().includes(q)) : lists
-  }, [lists, search])
+  const { lists, loading, search, setSearch, filtered, busyId, pick } = useSaveTripPlacesToList({
+    open: isOpen, tripId, placeIds, onClose, onDone,
+  })
 
   if (!isOpen) return null
 
-  const pick = async (list: Collection) => {
-    if (busyId != null || placeIds.length === 0) return
-    setBusyId(list.id)
-    try {
-      const res = await collectionsApi.saveFromTripMany(list.id, tripId, placeIds)
-      if (res.copied > 0) toast.success(t('collections.addedNToList', { count: res.copied, name: list.name }))
-      if (res.skipped.length > 0) toast.info(t('collections.skippedDuplicates', { count: res.skipped.length }))
-      if (res.copied === 0 && res.skipped.length === 0) toast.info(t('collections.copyNothing'))
-      onDone()
-      onClose()
-    } catch (err) {
-      toast.error(getApiErrorMessage(err, t('common.error')))
-    } finally {
-      setBusyId(null)
-    }
-  }
-
   return (
     <DialogShell
       onClose={onClose}

client/src/components/Collections/useSaveTripPlacesToList.test.ts

@@ -0,0 +1,226 @@
+// FE-COMP-SAVETRIPHOOK-001 to FE-COMP-SAVETRIPHOOK-010: the bulk save-to-list logic
+// behind both the desktop dialog and the phone sheet.
+import { act, renderHook, waitFor } from '@testing-library/react';
+import type { Collection, CollectionListResponse } from '@trek/shared';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+import { collectionsApi } from '../../api/collections';
+import { useSaveTripPlacesToList, type SaveTripPlacesToListOptions } from './useSaveTripPlacesToList';
+
+vi.mock('../../i18n', () => ({
+  useTranslation: () => ({
+    t: (k: string, p?: Record<string, unknown>) => (p ? `${k}:${JSON.stringify(p)}` : k),
+  }),
+}));
+
+function list(over: Partial<Collection>): Collection {
+  return { id: 1, owner_id: 7, name: 'List', color: null, place_count: 0, ...over } as Collection;
+}
+
+const FAVORITES = list({ id: 1, name: 'Favorites' });
+const WISHLIST = list({ id: 2, name: 'Wishlist' });
+const SHARED = list({ id: 3, name: 'Team ideas', is_owner: false });
+
+const listResponse = (collections: Collection[]): CollectionListResponse => ({ collections, incomingInvites: [] });
+
+let addToast: ReturnType<typeof vi.fn>;
+
+function setup(over: Partial<SaveTripPlacesToListOptions> = {}) {
+  const props: SaveTripPlacesToListOptions = {
+    open: true,
+    tripId: 5,
+    placeIds: [11, 12],
+    onClose: vi.fn(),
+    onDone: vi.fn(),
+    ...over,
+  };
+  const hook = renderHook((p: SaveTripPlacesToListOptions) => useSaveTripPlacesToList(p), { initialProps: props });
+  return { ...hook, props };
+}
+
+beforeEach(() => {
+  addToast = vi.fn();
+  window.__addToast = addToast as unknown as typeof window.__addToast;
+});
+
+afterEach(() => {
+  vi.restoreAllMocks();
+  delete window.__addToast;
+});
+
+describe('useSaveTripPlacesToList', () => {
+  it('FE-COMP-SAVETRIPHOOK-001: loads the writable lists on open and drops the read-only shares', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES, SHARED, WISHLIST]));
+    const { result } = setup();
+    expect(result.current.loading).toBe(true);
+    await waitFor(() => expect(result.current.loading).toBe(false));
+    expect(result.current.lists.map((l) => l.id)).toEqual([1, 2]);
+    expect(result.current.filtered).toBe(result.current.lists);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-002: requests nothing while closed, and loads again on the next opening', async () => {
+    const spy = vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES]));
+    const { result, rerender, props } = setup({ open: false });
+    expect(spy).not.toHaveBeenCalled();
+    expect(result.current.loading).toBe(false);
+
+    rerender({ ...props, open: true });
+    await waitFor(() => expect(result.current.lists).toHaveLength(1));
+    expect(spy).toHaveBeenCalledTimes(1);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-003: a missing or failing response leaves no lists', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue({ incomingInvites: [] } as unknown as CollectionListResponse);
+    const first = setup();
+    await waitFor(() => expect(first.result.current.loading).toBe(false));
+    expect(first.result.current.lists).toEqual([]);
+    first.unmount();
+
+    vi.spyOn(collectionsApi, 'list').mockRejectedValue(new Error('offline'));
+    const second = setup();
+    await waitFor(() => expect(second.result.current.loading).toBe(false));
+    expect(second.result.current.lists).toEqual([]);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-004: an answer that arrives after the picker closed is dropped', async () => {
+    let resolve!: (v: CollectionListResponse) => void;
+    vi.spyOn(collectionsApi, 'list').mockReturnValue(
+      new Promise((r) => {
+        resolve = r;
+      })
+    );
+    const { result, rerender, props } = setup();
+    rerender({ ...props, open: false });
+    await act(async () => {
+      resolve(listResponse([FAVORITES]));
+    });
+    expect(result.current.lists).toEqual([]);
+    // The cancelled load leaves its spinner flag alone, as before.
+    expect(result.current.loading).toBe(true);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-005: the search narrows by name, trimmed and case-insensitive, and an opening clears it', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES, WISHLIST]));
+    const { result, rerender, props } = setup();
+    await waitFor(() => expect(result.current.lists).toHaveLength(2));
+
+    act(() => result.current.setSearch('  WISH '));
+    expect(result.current.filtered.map((l) => l.name)).toEqual(['Wishlist']);
+
+    rerender({ ...props, open: false });
+    rerender({ ...props, open: true });
+    expect(result.current.search).toBe('');
+    await waitFor(() => expect(result.current.loading).toBe(false));
+    expect(result.current.filtered).toHaveLength(2);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-006: a pick copies the selection, reports it, then calls onDone before onClose', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES]));
+    const save = vi.spyOn(collectionsApi, 'saveFromTripMany').mockResolvedValue({ copied: 2, skipped: [] });
+    const { result, props } = setup();
+    await waitFor(() => expect(result.current.loading).toBe(false));
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+
+    expect(save).toHaveBeenCalledWith(1, 5, [11, 12]);
+    expect(addToast).toHaveBeenCalledWith(
+      'collections.addedNToList:{"count":2,"name":"Favorites"}',
+      'success',
+      undefined
+    );
+    expect(addToast).toHaveBeenCalledTimes(1);
+    expect(vi.mocked(props.onDone).mock.invocationCallOrder[0]).toBeLessThan(
+      vi.mocked(props.onClose).mock.invocationCallOrder[0]
+    );
+    expect(result.current.busyId).toBeNull();
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-007: duplicates the server skipped are reported, and a no-op save says so', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES]));
+    const save = vi
+      .spyOn(collectionsApi, 'saveFromTripMany')
+      .mockResolvedValueOnce({ copied: 0, skipped: [{ id: 12, name: 'Louvre' }] })
+      .mockResolvedValueOnce({ copied: 0, skipped: [] });
+    const { result } = setup();
+    await waitFor(() => expect(result.current.loading).toBe(false));
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+    expect(addToast).toHaveBeenCalledWith('collections.skippedDuplicates:{"count":1}', 'info', undefined);
+    expect(addToast).not.toHaveBeenCalledWith('collections.copyNothing', 'info', undefined);
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+    expect(save).toHaveBeenCalledTimes(2);
+    expect(addToast).toHaveBeenLastCalledWith('collections.copyNothing', 'info', undefined);
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-008: a failed save shows the server message and keeps the picker open', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES]));
+    vi.spyOn(collectionsApi, 'saveFromTripMany')
+      .mockRejectedValueOnce({ response: { data: { error: 'List is full' } } })
+      .mockRejectedValueOnce(new Error('network'));
+    const { result, props } = setup();
+    await waitFor(() => expect(result.current.loading).toBe(false));
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+    expect(addToast).toHaveBeenCalledWith('List is full', 'error', undefined);
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+    expect(addToast).toHaveBeenLastCalledWith('common.error', 'error', undefined);
+    expect(props.onDone).not.toHaveBeenCalled();
+    expect(props.onClose).not.toHaveBeenCalled();
+    expect(result.current.busyId).toBeNull();
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-009: a second pick while one is running is ignored', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES, WISHLIST]));
+    let release!: (v: { copied: number; skipped: { id: number; name: string }[] }) => void;
+    const save = vi.spyOn(collectionsApi, 'saveFromTripMany').mockReturnValue(
+      new Promise((r) => {
+        release = r;
+      })
+    );
+    const { result } = setup();
+    await waitFor(() => expect(result.current.loading).toBe(false));
+
+    let first!: Promise<void>;
+    act(() => {
+      first = result.current.pick(FAVORITES);
+    });
+    expect(result.current.busyId).toBe(1);
+
+    await act(async () => {
+      await result.current.pick(WISHLIST);
+    });
+    expect(save).toHaveBeenCalledTimes(1);
+
+    await act(async () => {
+      release({ copied: 1, skipped: [] });
+      await first;
+    });
+    expect(result.current.busyId).toBeNull();
+  });
+
+  it('FE-COMP-SAVETRIPHOOK-010: an empty selection sends nothing', async () => {
+    vi.spyOn(collectionsApi, 'list').mockResolvedValue(listResponse([FAVORITES]));
+    const save = vi.spyOn(collectionsApi, 'saveFromTripMany');
+    const { result, props } = setup({ placeIds: [] });
+    await waitFor(() => expect(result.current.loading).toBe(false));
+
+    await act(async () => {
+      await result.current.pick(FAVORITES);
+    });
+    expect(save).not.toHaveBeenCalled();
+    expect(props.onDone).not.toHaveBeenCalled();
+    expect(result.current.busyId).toBeNull();
+  });
+});

client/src/components/Collections/useSaveTripPlacesToList.ts

@@ -0,0 +1,80 @@
+import type { Collection } from '@trek/shared';
+import { useEffect, useMemo, useState } from 'react';
+
+import { collectionsApi } from '../../api/collections';
+import { useTranslation } from '../../i18n';
+import { getApiErrorMessage } from '../../utils/apiError';
+import { useToast } from '../shared/Toast';
+
+export interface SaveTripPlacesToListOptions {
+  /** Whether the picker is showing: every opening loads the lists afresh. */
+  open: boolean;
+  tripId: number;
+  /** The selected trip place ids to copy into the chosen list. */
+  placeIds: number[];
+  onClose: () => void;
+  /** Called after a successful save (e.g. to clear the trip selection). */
+  onDone: () => void;
+}
+
+/**
+ * Bulk "save to collection" for the trip's place selection: the one logic path behind
+ * the desktop dialog and the phone sheet, which render their own markup over it. Every
+ * opening loads the lists the user can add to, the search narrows them by name, and a
+ * pick copies every selected place into that list at once (the server dedups).
+ */
+export function useSaveTripPlacesToList({ open, tripId, placeIds, onClose, onDone }: SaveTripPlacesToListOptions) {
+  const { t } = useTranslation();
+  const toast = useToast();
+  const [lists, setLists] = useState<Collection[]>([]);
+  const [loading, setLoading] = useState(false);
+  const [search, setSearch] = useState('');
+  const [busyId, setBusyId] = useState<number | null>(null);
+
+  useEffect(() => {
+    if (!open) return;
+    let cancelled = false;
+    setLoading(true);
+    setSearch('');
+    collectionsApi
+      .list()
+      // Only lists the user can add to (their own or an editor/admin share). The
+      // server still enforces this; here we drop lists that are clearly read-only.
+      .then((res) => {
+        if (!cancelled) setLists((res.collections ?? []).filter((c) => c.is_owner !== false));
+      })
+      .catch(() => {
+        if (!cancelled) setLists([]);
+      })
+      .finally(() => {
+        if (!cancelled) setLoading(false);
+      });
+    return () => {
+      cancelled = true;
+    };
+  }, [open]);
+
+  const filtered = useMemo(() => {
+    const q = search.trim().toLowerCase();
+    return q ? lists.filter((l) => l.name.toLowerCase().includes(q)) : lists;
+  }, [lists, search]);
+
+  const pick = async (list: Collection) => {
+    if (busyId != null || placeIds.length === 0) return;
+    setBusyId(list.id);
+    try {
+      const res = await collectionsApi.saveFromTripMany(list.id, tripId, placeIds);
+      if (res.copied > 0) toast.success(t('collections.addedNToList', { count: res.copied, name: list.name }));
+      if (res.skipped.length > 0) toast.info(t('collections.skippedDuplicates', { count: res.skipped.length }));
+      if (res.copied === 0 && res.skipped.length === 0) toast.info(t('collections.copyNothing'));
+      onDone();
+      onClose();
+    } catch (err) {
+      toast.error(getApiErrorMessage(err, t('common.error')));
+    } finally {
+      setBusyId(null);
+    }
+  };
+
+  return { lists, loading, search, setSearch, filtered, busyId, pick };
+}

client/src/mobile/screens/trip/places/MPlacesSaveToCollectionSheet.tsx

@@ -1,12 +1,8 @@
-import { useEffect, useMemo, useState } from 'react'
 import { ArrowRight, Bookmark, Loader2, Plus, X } from 'lucide-react'
 import MSheet from '../../../components/MSheet'
 import MIconBtn from '../../../components/MIconBtn'
 import { useTranslation } from '../../../../i18n'
-import { useToast } from '../../../../components/shared/Toast'
-import { collectionsApi } from '../../../../api/collections'
-import { getApiErrorMessage } from '../../../../utils/apiError'
-import type { Collection } from '@trek/shared'
+import { useSaveTripPlacesToList } from '../../../../components/Collections/useSaveTripPlacesToList'
 
 interface MPlacesSaveToCollectionSheetProps {
   open: boolean
@@ -24,46 +20,9 @@ interface MPlacesSaveToCollectionSheetProps {
  */
 export default function MPlacesSaveToCollectionSheet({ open, tripId, placeIds, onClose, onDone }: MPlacesSaveToCollectionSheetProps) {
   const { t } = useTranslation()
-  const toast = useToast()
-  const [lists, setLists] = useState<Collection[]>([])
-  const [loading, setLoading] = useState(false)
-  const [search, setSearch] = useState('')
-  const [busyId, setBusyId] = useState<number | null>(null)
-
-  useEffect(() => {
-    if (!open) return
-    let cancelled = false
-    setLoading(true)
-    setSearch('')
-    collectionsApi.list()
-      // Only lists the user can add to; the server still enforces this.
-      .then(res => { if (!cancelled) setLists((res.collections ?? []).filter(c => c.is_owner !== false)) })
-      .catch(() => { if (!cancelled) setLists([]) })
-      .finally(() => { if (!cancelled) setLoading(false) })
-    return () => { cancelled = true }
-  }, [open])
-
-  const filtered = useMemo(() => {
-    const q = search.trim().toLowerCase()
-    return q ? lists.filter(l => l.name.toLowerCase().includes(q)) : lists
-  }, [lists, search])
-
-  const pick = async (list: Collection) => {
-    if (busyId != null || placeIds.length === 0) return
-    setBusyId(list.id)
-    try {
-      const res = await collectionsApi.saveFromTripMany(list.id, tripId, placeIds)
-      if (res.copied > 0) toast.success(t('collections.addedNToList', { count: res.copied, name: list.name }))
-      if (res.skipped.length > 0) toast.info(t('collections.skippedDuplicates', { count: res.skipped.length }))
-      if (res.copied === 0 && res.skipped.length === 0) toast.info(t('collections.copyNothing'))
-      onDone()
-      onClose()
-    } catch (err) {
-      toast.error(getApiErrorMessage(err, t('common.error')))
-    } finally {
-      setBusyId(null)
-    }
-  }
+  const { lists, loading, search, setSearch, filtered, busyId, pick } = useSaveTripPlacesToList({
+    open, tripId, placeIds, onClose, onDone,
+  })
 
   return (
     <MSheet open={open} onClose={onClose} variant="card" ariaLabel={t('collections.saveNToList', { count: placeIds.length })}>