From b2fd0ae5aa9008baf6d233b7252d95da8d8c49c3 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 21:09:18 +0000 Subject: [PATCH] refactor(collections): share the add place to list logic between desktop and phone useAddPlaceToCollection holds the maps search, the pick and the save that AddPlaceToCollectionModal and MCollAddSheet each carried, with the pure helpers in addPlaceModel. The differences stay as variants. The desktop resets and stays open after an add, sends links and typed coordinates and shows the error's own text. The phone closes, takes coordinates only from the picked hit, ignores a second search or save while one is running and shows the fallback text. --- .../Collections/AddPlaceToCollectionModal.tsx | 133 +---- .../Collections/addPlaceModel.test.ts | 25 + .../components/Collections/addPlaceModel.ts | 9 + .../useAddPlaceToCollection.test.ts | 476 ++++++++++++++++++ .../Collections/useAddPlaceToCollection.ts | 258 ++++++++++ .../screens/collections/MCollAddSheet.tsx | 103 +--- 6 files changed, 791 insertions(+), 213 deletions(-) create mode 100644 client/src/components/Collections/addPlaceModel.test.ts create mode 100644 client/src/components/Collections/addPlaceModel.ts create mode 100644 client/src/components/Collections/useAddPlaceToCollection.test.ts create mode 100644 client/src/components/Collections/useAddPlaceToCollection.ts diff --git a/client/src/components/Collections/AddPlaceToCollectionModal.tsx b/client/src/components/Collections/AddPlaceToCollectionModal.tsx index ce4776e08..39d3c45a6 100644 --- a/client/src/components/Collections/AddPlaceToCollectionModal.tsx +++ b/client/src/components/Collections/AddPlaceToCollectionModal.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useId, useRef, useState } from 'react' +import React, { useId, useRef } from 'react' import Markdown from 'react-markdown' import remarkGfm from 'remark-gfm' import remarkBreaks from 'remark-breaks' @@ -8,20 +8,12 @@ import { AddRowButton, EditorField, GRID_2, INPUT, PANEL, Segmented, TEXTAREA } import { NumericInput } from '../shared/NumericInput' import NoteFormatToolbar from '../shared/NoteFormatToolbar' import { Tooltip } from '../shared/Tooltip' -import { mapsApi } from '../../api/client' -import { collectionsApi } from '../../api/collections' import { getCategoryIcon } from '../shared/categoryIcons' -import { useTranslation } from '../../i18n' -import { useToast } from '../shared/Toast' -import { getApiErrorMessage } from '../../types' import { normalizeLinkUrl, STATUS_META, STATUS_ORDER } from '../../pages/collections/collectionsModel' import type { Category, TranslationFn } from '../../types' -import type { CollectionLink, CollectionStatus } from '@trek/shared' -import { usePlaceLanguage } from '../../hooks/usePlaceLanguage' - -type MapsPlace = Record -const str = (v: unknown): string | undefined => (typeof v === 'string' && v ? v : undefined) -const num = (v: unknown): number | undefined => (typeof v === 'number' ? v : typeof v === 'string' && v !== '' ? Number(v) : undefined) +import type { CollectionStatus } from '@trek/shared' +import { str } from './addPlaceModel' +import { useAddPlaceToCollection } from './useAddPlaceToCollection' /** * The search row, pinned to the top of the scrolling body. The dialog stays open @@ -54,114 +46,23 @@ interface AddPlaceToCollectionModalProps { * added in a row. */ export default function AddPlaceToCollectionModal({ isOpen, collectionId, collectionName, categories, onClose, onAdded, t }: AddPlaceToCollectionModalProps): React.ReactElement { - const { language } = useTranslation() - const placeLang = usePlaceLanguage() - const toast = useToast() - const [query, setQuery] = useState('') - const [results, setResults] = useState([]) - const [searching, setSearching] = useState(false) - // A search that came back empty used to render nothing at all, which reads as - // "the dialog is dead". Say so instead (#1921). - const [noResults, setNoResults] = useState(false) - // The picked location (address/coords/ids) plus the editable fields. - const [picked, setPicked] = useState(null) - const [name, setName] = useState('') - // Address + coordinates: prefilled from a picked result, but also directly - // typeable so a place can be added by GPS without searching (#1435). - const [address, setAddress] = useState('') - const [lat, setLat] = useState('') - const [lng, setLng] = useState('') - const [categoryId, setCategoryId] = useState(null) - const [description, setDescription] = useState('') - const [links, setLinks] = useState([]) - const [status, setStatus] = useState('idea') - const [saving, setSaving] = useState(false) - const descRef = useRef(null) const searchRef = useRef(null) + const { + query, setQuery, results, searching, noResults, setNoResults, dismissResults, name, setName, address, setAddress, + lat, setLat, lng, setLng, categoryId, setCategoryId, description, setDescription, links, setLinks, setLink, + status, setStatus, saving, search, pick, coordPaste, save, + } = useAddPlaceToCollection({ + variant: 'dialog', open: isOpen, collectionId, collectionName, t, onClose, onAdded, normalizeLinkUrl, + // The dialog stays open for the next place, so hand the caret back to the + // search field. The add button the user just clicked goes disabled with the + // cleared name, which drops the focus to and leaves the dialog dead + // to the keyboard (#1921). + afterAdd: () => searchRef.current?.focus(), + }) + const descRef = useRef(null) const labelId = useId() const fieldId = useId() - const reset = () => { setQuery(''); setResults([]); setNoResults(false); setPicked(null); setName(''); setAddress(''); setLat(''); setLng(''); setCategoryId(null); setDescription(''); setLinks([]); setStatus('idea') } - useEffect(() => { if (!isOpen) reset() }, [isOpen]) - - const search = async () => { - if (!query.trim()) return - setSearching(true) - setNoResults(false) - try { - const res = await mapsApi.search(query, placeLang) - const places = (res.places as MapsPlace[]) || [] - setResults(places) - setNoResults(places.length === 0) - } catch (err) { - toast.error(getApiErrorMessage(err, t('places.mapsSearchError'))) - } finally { - setSearching(false) - } - } - - const dismissResults = () => { setResults([]); setNoResults(false) } - - const pick = (r: MapsPlace) => { - setPicked(r) - setName(str(r.name) ?? '') - setAddress(str(r.address) ?? '') - const la = num(r.lat); const lo = num(r.lng) - setLat(la != null ? String(la) : '') - setLng(lo != null ? String(lo) : '') - setResults([]); setNoResults(false); setQuery(str(r.name) ?? query) - } - const setLink = (i: number, patch: Partial) => setLinks(links.map((l, idx) => (idx === i ? { ...l, ...patch } : l))) - - const save = async () => { - const cleanName = name.trim() - if (!cleanName) return - const cleanLinks = links.map(l => ({ label: l.label?.trim() || undefined, url: normalizeLinkUrl(l.url) })).filter(l => l.url) - const latNum = lat.trim() ? Number(lat) : Number.NaN - const lngNum = lng.trim() ? Number(lng) : Number.NaN - setSaving(true) - try { - const res = await collectionsApi.savePlace({ - collection_id: collectionId, - name: cleanName, - address: address.trim() || null, - lat: Number.isFinite(latNum) ? latNum : null, - lng: Number.isFinite(lngNum) ? lngNum : null, - google_place_id: (picked && str(picked.google_place_id)) ?? null, - google_ftid: (picked && str(picked.google_ftid)) ?? null, - osm_id: (picked && str(picked.osm_id)) ?? null, - website: (picked && str(picked.website)) ?? null, - phone: (picked && str(picked.phone)) ?? null, - category_id: categoryId, - description: description.trim() || null, - links: cleanLinks, - status, - force: true, - }) - if (res.duplicate) toast.info(t('collections.duplicateWarning')) - else { toast.success(t('collections.addedToList', { name: collectionName })); onAdded() } - reset() - // The dialog stays open for the next place, so hand the caret back to the - // search field. The add button the user just clicked goes disabled with the - // cleared name, which drops the focus to and leaves the dialog dead - // to the keyboard (#1921). - searchRef.current?.focus() - } catch (err) { - toast.error(getApiErrorMessage(err, t('common.error'))) - } finally { - setSaving(false) - } - } - - const coordPaste = (e: React.ClipboardEvent) => { - const text = e.clipboardData.getData('text').trim() - // Same pairs as before, written so no two quantifiers can claim the same - // character. In the old form `\d+\.?\d*` and `\s*[,;\s]\s*` were both ambiguous, - // which backtracks in O(n^4): a pasted 2 kB of digits and spaces froze the tab. - const match = text.match(/^(-?\d+(?:\.\d*)?)(?:\s*[,;]\s*|\s+)(-?\d+(?:\.\d*)?)$/) - if (match) { e.preventDefault(); setLat(match[1]); setLng(match[2]) } - } - const header = ( } diff --git a/client/src/components/Collections/addPlaceModel.test.ts b/client/src/components/Collections/addPlaceModel.test.ts new file mode 100644 index 000000000..92ec5160e --- /dev/null +++ b/client/src/components/Collections/addPlaceModel.test.ts @@ -0,0 +1,25 @@ +// FE-COMP-ADDPLACEMODEL-001 to FE-COMP-ADDPLACEMODEL-002: reading the loosely typed +// fields of a maps search hit. +import { describe, expect, it } from 'vitest'; + +import { num, str } from './addPlaceModel'; + +describe('addPlaceModel', () => { + it('FE-COMP-ADDPLACEMODEL-001: str keeps a non-empty string and drops everything else', () => { + expect(str('Elbphilharmonie')).toBe('Elbphilharmonie'); + expect(str('')).toBeUndefined(); + expect(str(42)).toBeUndefined(); + expect(str(null)).toBeUndefined(); + expect(str(undefined)).toBeUndefined(); + }); + + it('FE-COMP-ADDPLACEMODEL-002: num reads a number or numeric text, and nothing else', () => { + expect(num(9.9841)).toBe(9.9841); + expect(num(0)).toBe(0); + expect(num('53.5413')).toBe(53.5413); + expect(num('')).toBeUndefined(); + expect(num(null)).toBeUndefined(); + expect(num(undefined)).toBeUndefined(); + expect(Number.isNaN(num('abc'))).toBe(true); + }); +}); diff --git a/client/src/components/Collections/addPlaceModel.ts b/client/src/components/Collections/addPlaceModel.ts new file mode 100644 index 000000000..3387c5374 --- /dev/null +++ b/client/src/components/Collections/addPlaceModel.ts @@ -0,0 +1,9 @@ +/** A place as the maps search hands it back: loosely typed, every field optional. */ +export type MapsPlace = Record; + +/** A non-empty string field of a maps place, or undefined. */ +export const str = (v: unknown): string | undefined => (typeof v === 'string' && v ? v : undefined); + +/** A numeric field of a maps place, whether it came as a number or as text, or undefined. */ +export const num = (v: unknown): number | undefined => + typeof v === 'number' ? v : typeof v === 'string' && v !== '' ? Number(v) : undefined; diff --git a/client/src/components/Collections/useAddPlaceToCollection.test.ts b/client/src/components/Collections/useAddPlaceToCollection.test.ts new file mode 100644 index 000000000..85d6d0f2d --- /dev/null +++ b/client/src/components/Collections/useAddPlaceToCollection.test.ts @@ -0,0 +1,476 @@ +// FE-COMP-ADDPLACEHOOK-001 to FE-COMP-ADDPLACEHOOK-020: the add-a-place logic behind the +// desktop dialog (variant 'dialog') and the phone sheet ('sheet'). +import { act, renderHook, waitFor } from '@testing-library/react'; +import type React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { resetAllStores } from '../../../tests/helpers/store'; +import { mapsApi } from '../../api/client'; +import { collectionsApi } from '../../api/collections'; +import { useAddPlaceToCollection, type AddPlaceToCollectionOptions } from './useAddPlaceToCollection'; + +type SearchResult = Awaited>; +type SaveResult = Awaited>; + +const t = (key: string, params?: Record) => + params ? `${key}:${JSON.stringify(params)}` : key; + +/** A search hit shaped like the maps proxy: lat arrives as text, website empty. */ +const HIT = { + name: 'Elbphilharmonie', + address: 'Platz der Deutschen Einheit 1', + lat: '53.5413', + lng: 9.9841, + google_place_id: 'gp-1', + google_ftid: 'ft-1', + osm_id: 'osm-1', + website: '', + phone: '+49 40 357666', +}; + +const LISTS = [ + { id: 1, name: 'Hamburg' }, + { id: 2, name: 'Berlin' }, +]; + +let addToast: ReturnType; + +const normalizeLinkUrl = (url: string) => (url.trim() ? `https://${url.trim()}` : ''); + +function dialog(over: Partial<{ open: boolean; afterAdd: () => void }> = {}) { + const onClose = vi.fn(); + const onAdded = vi.fn(); + const afterAdd = over.afterAdd ?? vi.fn(); + const hook = renderHook( + (p: { open: boolean }) => + useAddPlaceToCollection({ + variant: 'dialog', + open: p.open, + collectionId: 9, + collectionName: 'Hamburg', + t, + onClose, + onAdded, + normalizeLinkUrl, + afterAdd, + }), + { initialProps: { open: over.open ?? true } } + ); + return { ...hook, onClose, onAdded, afterAdd }; +} + +type SheetProps = Pick, 'open' | 'collectionId' | 'lists'>; + +function sheet(initial: Partial = {}) { + const onClose = vi.fn(); + const onAdded = vi.fn(); + const hook = renderHook( + (p: SheetProps) => + useAddPlaceToCollection({ variant: 'sheet', ...p, collectionName: 'All saved', t, onClose, onAdded }), + { initialProps: { open: true, collectionId: 1, lists: LISTS, ...initial } } + ); + return { ...hook, onClose, onAdded }; +} + +function mockSearch(places: Record[] = [HIT]) { + return vi.spyOn(mapsApi, 'search').mockResolvedValue({ places, source: 'osm' } as unknown as SearchResult); +} + +function mockSave(result: Partial = {}) { + return vi.spyOn(collectionsApi, 'savePlace').mockResolvedValue(result as SaveResult); +} + +type Hook = { current: ReturnType }; + +async function searchFor(result: Hook, query: string) { + act(() => result.current.setQuery(query)); + await act(async () => { + await result.current.search(); + }); +} + +async function saveNow(result: Hook) { + await act(async () => { + await result.current.save(); + }); +} + +beforeEach(() => { + resetAllStores(); + addToast = vi.fn(); + window.__addToast = addToast as unknown as typeof window.__addToast; +}); + +afterEach(() => { + vi.restoreAllMocks(); + delete window.__addToast; +}); + +describe('useAddPlaceToCollection: searching', () => { + it('FE-COMP-ADDPLACEHOOK-001: a blank query searches nothing', async () => { + const spy = mockSearch(); + const { result } = dialog(); + await searchFor(result, ' '); + expect(spy).not.toHaveBeenCalled(); + }); + + it('FE-COMP-ADDPLACEHOOK-002: the dialog searches in the place language and says when nothing was found', async () => { + const spy = mockSearch([]); + const { result } = dialog(); + await searchFor(result, 'Elbphilharmonie'); + expect(spy).toHaveBeenCalledWith('Elbphilharmonie', 'en'); + expect(result.current.results).toEqual([]); + expect(result.current.noResults).toBe(true); + expect(result.current.searching).toBe(false); + + spy.mockResolvedValue({ places: [HIT], source: 'osm' } as unknown as SearchResult); + await searchFor(result, 'Elbphilharmonie'); + expect(result.current.results).toEqual([HIT]); + expect(result.current.noResults).toBe(false); + + act(() => result.current.dismissResults()); + expect(result.current.results).toEqual([]); + }); + + it('FE-COMP-ADDPLACEHOOK-003: the sheet never reports an empty search', async () => { + mockSearch([]); + const { result } = sheet(); + await searchFor(result, 'nowhere'); + expect(result.current.noResults).toBe(false); + }); + + it('FE-COMP-ADDPLACEHOOK-004: a failed search shows the server message on both', async () => { + vi.spyOn(mapsApi, 'search').mockRejectedValue({ response: { data: { error: 'Quota exceeded' } } }); + const d = dialog(); + await searchFor(d.result, 'Elb'); + expect(addToast).toHaveBeenLastCalledWith('Quota exceeded', 'error', undefined); + d.unmount(); + const s = sheet(); + await searchFor(s.result, 'Elb'); + expect(addToast).toHaveBeenLastCalledWith('Quota exceeded', 'error', undefined); + }); + + it("FE-COMP-ADDPLACEHOOK-005: without a server message the dialog shows the error's own text, the sheet the fallback", async () => { + vi.spyOn(mapsApi, 'search').mockRejectedValue(new Error('Network Error')); + const d = dialog(); + await searchFor(d.result, 'Elb'); + expect(addToast).toHaveBeenLastCalledWith('Network Error', 'error', undefined); + d.unmount(); + const s = sheet(); + await searchFor(s.result, 'Elb'); + expect(addToast).toHaveBeenLastCalledWith('places.mapsSearchError', 'error', undefined); + }); + + it('FE-COMP-ADDPLACEHOOK-006: the sheet does not start a second search while one runs; the dialog does', async () => { + const releases: Array<(v: SearchResult) => void> = []; + const spy = vi.spyOn(mapsApi, 'search').mockImplementation( + () => + new Promise((resolve) => { + releases.push(resolve); + }) + ); + const empty = { places: [], source: 'osm' } as unknown as SearchResult; + + const s = sheet(); + act(() => s.result.current.setQuery('Elb')); + let first!: Promise; + act(() => { + first = s.result.current.search(); + }); + expect(s.result.current.searching).toBe(true); + await act(async () => { + await s.result.current.search(); + }); + expect(spy).toHaveBeenCalledTimes(1); + await act(async () => { + releases.forEach((release) => release(empty)); + await first; + }); + s.unmount(); + + const d = dialog(); + act(() => d.result.current.setQuery('Elb')); + let second!: Promise; + act(() => { + first = d.result.current.search(); + }); + expect(d.result.current.searching).toBe(true); + act(() => { + second = d.result.current.search(); + }); + expect(spy).toHaveBeenCalledTimes(3); + await act(async () => { + releases.forEach((release) => release(empty)); + await Promise.all([first, second]); + }); + expect(d.result.current.searching).toBe(false); + }); +}); + +describe('useAddPlaceToCollection: picking', () => { + it('FE-COMP-ADDPLACEHOOK-007: the dialog takes name, address and coordinates from the hit', () => { + const { result } = dialog(); + act(() => result.current.setQuery('elb')); + act(() => result.current.pick(HIT)); + expect(result.current.name).toBe('Elbphilharmonie'); + expect(result.current.address).toBe('Platz der Deutschen Einheit 1'); + expect(result.current.lat).toBe('53.5413'); + expect(result.current.lng).toBe('9.9841'); + expect(result.current.query).toBe('Elbphilharmonie'); + expect(result.current.results).toEqual([]); + }); + + it('FE-COMP-ADDPLACEHOOK-008: a hit without name or coordinates leaves those blank and keeps the query', () => { + const { result } = dialog(); + act(() => result.current.setQuery('elb')); + act(() => result.current.pick({ address: 'Somewhere' })); + expect(result.current.name).toBe(''); + expect(result.current.lat).toBe(''); + expect(result.current.lng).toBe(''); + expect(result.current.query).toBe('elb'); + }); + + it('FE-COMP-ADDPLACEHOOK-009: the sheet takes name and address but leaves the coordinates fields alone', () => { + const { result } = sheet(); + act(() => result.current.pick(HIT)); + expect(result.current.name).toBe('Elbphilharmonie'); + expect(result.current.address).toBe('Platz der Deutschen Einheit 1'); + expect(result.current.lat).toBe(''); + expect(result.current.lng).toBe(''); + }); + + it('FE-COMP-ADDPLACEHOOK-010: a pasted coordinate pair fills both fields, anything else is left to the input', () => { + const { result } = dialog(); + const preventDefault = vi.fn(); + const paste = (text: string) => + ({ clipboardData: { getData: () => text }, preventDefault }) as unknown as React.ClipboardEvent; + act(() => result.current.coordPaste(paste(' 53.55, 9.99 '))); + expect(result.current.lat).toBe('53.55'); + expect(result.current.lng).toBe('9.99'); + expect(preventDefault).toHaveBeenCalledTimes(1); + act(() => result.current.coordPaste(paste('Hamburg'))); + expect(preventDefault).toHaveBeenCalledTimes(1); + expect(result.current.lat).toBe('53.55'); + }); +}); + +describe('useAddPlaceToCollection: saving from the dialog', () => { + it('FE-COMP-ADDPLACEHOOK-011: a blank name saves nothing', async () => { + const save = mockSave(); + const { result } = dialog(); + act(() => result.current.setName(' ')); + await saveNow(result); + expect(save).not.toHaveBeenCalled(); + }); + + it('FE-COMP-ADDPLACEHOOK-012: posts the typed place with its links, then resets for the next one', async () => { + const save = mockSave({ duplicate: false }); + const { result, onAdded, onClose, afterAdd } = dialog(); + act(() => result.current.pick(HIT)); + act(() => { + result.current.setName(' Elphi '); + result.current.setLat('53.5'); + result.current.setLng(''); + result.current.setCategoryId(4); + result.current.setDescription(' Concerts '); + result.current.setStatus('visited'); + result.current.setLinks([{ url: 'tickets.example' }, { url: ' ', label: 'blank' }]); + }); + act(() => result.current.setLink(0, { label: ' Tickets ' })); + + await saveNow(result); + + expect(save).toHaveBeenCalledWith({ + collection_id: 9, + name: 'Elphi', + address: 'Platz der Deutschen Einheit 1', + lat: 53.5, + lng: null, + google_place_id: 'gp-1', + google_ftid: 'ft-1', + osm_id: 'osm-1', + website: null, + phone: '+49 40 357666', + category_id: 4, + description: 'Concerts', + links: [{ label: 'Tickets', url: 'https://tickets.example' }], + status: 'visited', + force: true, + }); + expect(addToast).toHaveBeenCalledWith('collections.addedToList:{"name":"Hamburg"}', 'success', undefined); + expect(onAdded).toHaveBeenCalledTimes(1); + expect(onClose).not.toHaveBeenCalled(); + expect(afterAdd).toHaveBeenCalledTimes(1); + expect(result.current.name).toBe(''); + expect(result.current.links).toEqual([]); + expect(result.current.status).toBe('idea'); + expect(result.current.saving).toBe(false); + }); + + it('FE-COMP-ADDPLACEHOOK-013: a hand-typed place carries no provenance, and an unparsable coordinate goes as null', async () => { + const save = mockSave(); + const { result } = dialog(); + act(() => { + result.current.setName('Corner shop'); + result.current.setLat('abc'); + result.current.setLng('10'); + }); + await saveNow(result); + expect(save).toHaveBeenCalledWith( + expect.objectContaining({ + address: null, + lat: null, + lng: 10, + google_place_id: null, + osm_id: null, + description: null, + links: [], + status: 'idea', + }) + ); + }); + + it('FE-COMP-ADDPLACEHOOK-014: a duplicate informs without counting as added, and the form still resets', async () => { + mockSave({ duplicate: true }); + const { result, onAdded, afterAdd } = dialog(); + act(() => result.current.setName('Elphi')); + await saveNow(result); + expect(addToast).toHaveBeenCalledWith('collections.duplicateWarning', 'info', undefined); + expect(onAdded).not.toHaveBeenCalled(); + expect(afterAdd).toHaveBeenCalledTimes(1); + expect(result.current.name).toBe(''); + }); + + it('FE-COMP-ADDPLACEHOOK-015: a failed save keeps the form, and a plain error shows its own text', async () => { + vi.spyOn(collectionsApi, 'savePlace') + .mockRejectedValueOnce({ response: { data: { error: 'List is full' } } }) + .mockRejectedValueOnce(new Error('Network Error')); + const { result, afterAdd } = dialog(); + act(() => result.current.setName('Elphi')); + await saveNow(result); + expect(addToast).toHaveBeenLastCalledWith('List is full', 'error', undefined); + await saveNow(result); + expect(addToast).toHaveBeenLastCalledWith('Network Error', 'error', undefined); + expect(result.current.name).toBe('Elphi'); + expect(afterAdd).not.toHaveBeenCalled(); + expect(result.current.saving).toBe(false); + }); + + it('FE-COMP-ADDPLACEHOOK-016: closing the dialog resets every field', () => { + const { result, rerender } = dialog(); + act(() => { + result.current.setQuery('elb'); + result.current.pick(HIT); + result.current.setDescription('x'); + result.current.setLinks([{ url: 'a.example' }]); + result.current.setNoResults(true); + }); + rerender({ open: false }); + expect(result.current.query).toBe(''); + expect(result.current.name).toBe(''); + expect(result.current.lat).toBe(''); + expect(result.current.description).toBe(''); + expect(result.current.links).toEqual([]); + expect(result.current.noResults).toBe(false); + }); +}); + +describe('useAddPlaceToCollection: saving from the sheet', () => { + it('FE-COMP-ADDPLACEHOOK-017: posts the picked place without links and closes, naming the target list', async () => { + const save = mockSave({ duplicate: false }); + const { result, onAdded, onClose } = sheet({ collectionId: null, lists: LISTS }); + act(() => result.current.setTargetId(2)); + act(() => result.current.pick(HIT)); + await saveNow(result); + expect(save).toHaveBeenCalledWith({ + collection_id: 2, + name: 'Elbphilharmonie', + address: 'Platz der Deutschen Einheit 1', + lat: 53.5413, + lng: 9.9841, + google_place_id: 'gp-1', + google_ftid: 'ft-1', + osm_id: 'osm-1', + website: null, + phone: '+49 40 357666', + category_id: null, + description: null, + status: 'idea', + force: true, + }); + expect(addToast).toHaveBeenCalledWith('collections.addedToList:{"name":"Berlin"}', 'success', undefined); + expect(onAdded).toHaveBeenCalledTimes(1); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('FE-COMP-ADDPLACEHOOK-018: without a target, or while saving, nothing is sent; a duplicate still closes', async () => { + let release: (v: SaveResult) => void = () => {}; + const save = vi.spyOn(collectionsApi, 'savePlace').mockImplementation( + () => + new Promise((resolve) => { + release = resolve; + }) + ); + const { result, onClose, onAdded } = sheet({ collectionId: null, lists: LISTS }); + act(() => result.current.setName('Elphi')); + await saveNow(result); + expect(save).not.toHaveBeenCalled(); + + act(() => result.current.setTargetId(1)); + let first!: Promise; + act(() => { + first = result.current.save(); + }); + expect(result.current.saving).toBe(true); + await saveNow(result); + expect(save).toHaveBeenCalledTimes(1); + await act(async () => { + release({ duplicate: true } as SaveResult); + await first; + }); + expect(onAdded).not.toHaveBeenCalled(); + expect(onClose).toHaveBeenCalledTimes(1); + // The sheet closes rather than resetting: the form is cleared when it closes. + expect(result.current.name).toBe('Elphi'); + }); + + it('FE-COMP-ADDPLACEHOOK-019: a failed save shows the fallback for a plain error and stays open', async () => { + vi.spyOn(collectionsApi, 'savePlace').mockRejectedValue(new Error('Network Error')); + const { result, onClose } = sheet(); + act(() => result.current.setName('Elphi')); + await saveNow(result); + expect(addToast).toHaveBeenCalledWith('common.error', 'error', undefined); + expect(onClose).not.toHaveBeenCalled(); + }); + + it('FE-COMP-ADDPLACEHOOK-020: the target follows a fixed list, defaults to a sole list, keeps a pick and clears on close', async () => { + const fixed = sheet({ collectionId: 2, lists: LISTS }); + expect(fixed.result.current.targetId).toBe(2); + fixed.unmount(); + + const { result, rerender } = sheet({ collectionId: null, lists: [] }); + expect(result.current.targetId).toBeNull(); + // The lists arrive after the opening: the sole one becomes the default. + rerender({ open: true, collectionId: null, lists: [LISTS[0]] }); + expect(result.current.targetId).toBe(1); + // A pick that is still among the lists stands when they change. + act(() => result.current.setTargetId(2)); + rerender({ open: true, collectionId: null, lists: [...LISTS] }); + expect(result.current.targetId).toBe(2); + // Several lists and no valid pick: the sheet waits for one. + rerender({ + open: true, + collectionId: null, + lists: [ + { id: 5, name: 'Rome' }, + { id: 6, name: 'Paris' }, + ], + }); + expect(result.current.targetId).toBeNull(); + + act(() => result.current.setName('Elphi')); + rerender({ open: false, collectionId: null, lists: LISTS }); + await waitFor(() => expect(result.current.name).toBe('')); + expect(result.current.targetId).toBeNull(); + }); +}); diff --git a/client/src/components/Collections/useAddPlaceToCollection.ts b/client/src/components/Collections/useAddPlaceToCollection.ts new file mode 100644 index 000000000..5d31c6043 --- /dev/null +++ b/client/src/components/Collections/useAddPlaceToCollection.ts @@ -0,0 +1,258 @@ +import type { CollectionLink, CollectionStatus } from '@trek/shared'; +import type React from 'react'; +import { useEffect, useEffectEvent, useState } from 'react'; + +import { mapsApi } from '../../api/client'; +import { collectionsApi } from '../../api/collections'; +import { usePlaceLanguage } from '../../hooks/usePlaceLanguage'; +import { getApiErrorMessage as errorWithMessage, type TranslationFn } from '../../types'; +import { getApiErrorMessage } from '../../utils/apiError'; +import { parseCoordinatePair } from '../Planner/PlaceFormModal.helpers'; +import { useToast } from '../shared/Toast'; +import { type MapsPlace, num, str } from './addPlaceModel'; + +interface CommonOptions { + open: boolean; + /** The list the place goes into. On the phone null, when opened from "All saved": the list is picked in the sheet. */ + collectionId: number | null; + collectionName: string; + t: TranslationFn; + onClose: () => void; + onAdded: () => void; +} + +/** + * The two shells read the form differently, and each keeps its own reading. + * + * `dialog` (desktop): stays open after an add, so the form resets for the next place; + * the coordinates are typed or pasted, links ride along, a search that finds nothing + * says so, and an error without a server message shows the error's own text. + * + * `sheet` (phone): closes after an add, the target list may be picked in the sheet, + * the coordinates come from the picked hit only, a search or save already running is + * not started again, and an error without a server message shows the fallback. + */ +export type AddPlaceToCollectionOptions = CommonOptions & + ( + | { + variant: 'dialog'; + /** How a typed link becomes the stored one. */ + normalizeLinkUrl: (url: string) => string; + /** Runs after an add, once the form is reset for the next place. */ + afterAdd?: () => void; + } + | { + variant: 'sheet'; + /** The pickable target lists, for an opening without a fixed list. */ + lists: { id: number; name: string }[]; + } + ); + +const NO_LISTS: { id: number; name: string }[] = []; + +/** + * Adding a place to a collection list: the one logic path behind the desktop dialog and + * the phone sheet, which render their own markup over it. A maps search fills the name + * and the location when a hit is picked, the rest is typed, and the save goes to the + * list (duplicates are reported by the server and surface as a toast). + */ +export function useAddPlaceToCollection(options: AddPlaceToCollectionOptions) { + const { open, collectionId, collectionName, t, onClose, onAdded } = options; + const lists = options.variant === 'sheet' ? options.lists : NO_LISTS; + const sheet = options.variant === 'sheet'; + const placeLang = usePlaceLanguage(); + const toast = useToast(); + const [query, setQuery] = useState(''); + const [results, setResults] = useState([]); + const [searching, setSearching] = useState(false); + // A search that came back empty used to render nothing at all, which reads as + // "the dialog is dead". Say so instead (#1921). + const [noResults, setNoResults] = useState(false); + // The picked location (address/coords/ids) plus the editable fields. + const [picked, setPicked] = useState(null); + const [name, setName] = useState(''); + // Address + coordinates: prefilled from a picked result, but also directly + // typeable so a place can be added by GPS without searching (#1435). + const [address, setAddress] = useState(''); + const [lat, setLat] = useState(''); + const [lng, setLng] = useState(''); + const [categoryId, setCategoryId] = useState(null); + const [description, setDescription] = useState(''); + const [links, setLinks] = useState([]); + const [status, setStatus] = useState('idea'); + const [saving, setSaving] = useState(false); + const [targetId, setTargetId] = useState(collectionId); + + const errorText = sheet ? getApiErrorMessage : errorWithMessage; + + const reset = () => { + setQuery(''); + setResults([]); + setNoResults(false); + setPicked(null); + setName(''); + setAddress(''); + setLat(''); + setLng(''); + setCategoryId(null); + setDescription(''); + setLinks([]); + setStatus('idea'); + }; + + const resetOnClose = useEffectEvent(() => { + reset(); + if (sheet) setTargetId(null); + }); + useEffect(() => { + if (!open) resetOnClose(); + }, [open]); + + // Opened from a specific list → target is fixed; from "All Saved" (no active + // list) → default to the sole list if there is one, else pick it in-sheet. + // The lists may still be loading when the sheet opens, so the default is + // applied again once they arrive, without overruling a pick already made. + useEffect(() => { + if (!sheet || !open) return; + if (collectionId != null) { + setTargetId(collectionId); + return; + } + setTargetId((prev) => + prev != null && lists.some((l) => l.id === prev) ? prev : lists.length === 1 ? lists[0].id : null + ); + }, [sheet, open, collectionId, lists]); + + const search = async () => { + if (!query.trim() || (sheet && searching)) return; + setSearching(true); + if (!sheet) setNoResults(false); + try { + const res = await mapsApi.search(query, placeLang); + const places = (res.places as MapsPlace[]) || []; + setResults(places); + if (!sheet) setNoResults(places.length === 0); + } catch (err) { + toast.error(errorText(err, t('places.mapsSearchError'))); + } finally { + setSearching(false); + } + }; + + const dismissResults = () => { + setResults([]); + setNoResults(false); + }; + + const pick = (r: MapsPlace) => { + setPicked(r); + setName(str(r.name) ?? ''); + setAddress(str(r.address) ?? ''); + if (!sheet) { + const la = num(r.lat); + const lo = num(r.lng); + setLat(la != null ? String(la) : ''); + setLng(lo != null ? String(lo) : ''); + } + setResults([]); + if (!sheet) setNoResults(false); + setQuery(str(r.name) ?? query); + }; + + const setLink = (i: number, patch: Partial) => + setLinks(links.map((l, idx) => (idx === i ? { ...l, ...patch } : l))); + + /** "48.85, 2.35" pasted into either coordinate fills both halves. */ + const coordPaste = (e: React.ClipboardEvent) => { + const pair = parseCoordinatePair(e.clipboardData.getData('text')); + if (pair) { + e.preventDefault(); + setLat(pair[0]); + setLng(pair[1]); + } + }; + + const save = async () => { + const cleanName = name.trim(); + const target = sheet ? targetId : collectionId; + if (!cleanName || target == null || (sheet && saving)) return; + const latNum = lat.trim() ? Number(lat) : Number.NaN; + const lngNum = lng.trim() ? Number(lng) : Number.NaN; + setSaving(true); + try { + const res = await collectionsApi.savePlace({ + collection_id: target, + name: cleanName, + address: address.trim() || null, + lat: sheet ? ((picked && num(picked.lat)) ?? null) : Number.isFinite(latNum) ? latNum : null, + lng: sheet ? ((picked && num(picked.lng)) ?? null) : Number.isFinite(lngNum) ? lngNum : null, + google_place_id: (picked && str(picked.google_place_id)) ?? null, + google_ftid: (picked && str(picked.google_ftid)) ?? null, + osm_id: (picked && str(picked.osm_id)) ?? null, + website: (picked && str(picked.website)) ?? null, + phone: (picked && str(picked.phone)) ?? null, + category_id: categoryId, + description: description.trim() || null, + ...(options.variant === 'dialog' + ? { + links: links + .map((l) => ({ label: l.label?.trim() || undefined, url: options.normalizeLinkUrl(l.url) })) + .filter((l) => l.url), + } + : {}), + status, + force: true, + }); + if (res.duplicate) toast.info(t('collections.duplicateWarning')); + else { + const listName = sheet ? (lists.find((l) => l.id === target)?.name ?? collectionName) : collectionName; + toast.success(t('collections.addedToList', { name: listName })); + onAdded(); + } + if (options.variant === 'sheet') onClose(); + else { + reset(); + options.afterAdd?.(); + } + } catch (err) { + toast.error(errorText(err, t('common.error'))); + } finally { + setSaving(false); + } + }; + + return { + query, + setQuery, + results, + setResults, + searching, + noResults, + setNoResults, + dismissResults, + name, + setName, + address, + setAddress, + lat, + setLat, + lng, + setLng, + categoryId, + setCategoryId, + description, + setDescription, + links, + setLinks, + setLink, + status, + setStatus, + saving, + targetId, + setTargetId, + search, + pick, + coordPaste, + save, + }; +} diff --git a/client/src/mobile/screens/collections/MCollAddSheet.tsx b/client/src/mobile/screens/collections/MCollAddSheet.tsx index a0382ed40..20b80f332 100644 --- a/client/src/mobile/screens/collections/MCollAddSheet.tsx +++ b/client/src/mobile/screens/collections/MCollAddSheet.tsx @@ -1,22 +1,12 @@ -import { useEffect, useState } from 'react' import { Check, Loader2, MapPin, Search, X } from 'lucide-react' -import type { CollectionStatus } from '@trek/shared' import type { Category, TranslationFn } from '../../../types' -import { mapsApi } from '../../../api/client' -import { collectionsApi } from '../../../api/collections' -import { useTranslation } from '../../../i18n' -import { useToast } from '../../../components/shared/Toast' -import { getApiErrorMessage } from '../../../utils/apiError' import { STATUS_ORDER } from '../../../pages/collections/collectionsModel' import MSheet from '../../components/MSheet' import MCollCategoryPicker from './MCollCategoryPicker' import { STATUS_SPEC } from './collectionsMobileModel' import { CancelPill, Eyebrow, INPUT_CLS, PrimaryPill, SheetFooter, SheetHeader, TEXTAREA_CLS } from './MCollSheetKit' -import { usePlaceLanguage } from '../../../hooks/usePlaceLanguage' - -type MapsPlace = Record -const str = (v: unknown): string | undefined => (typeof v === 'string' && v ? v : undefined) -const num = (v: unknown): number | undefined => (typeof v === 'number' ? v : typeof v === 'string' && v !== '' ? Number(v) : undefined) +import { str } from '../../../components/Collections/addPlaceModel' +import { useAddPlaceToCollection } from '../../../components/Collections/useAddPlaceToCollection' interface MCollAddSheetProps { open: boolean @@ -36,91 +26,10 @@ interface MCollAddSheetProps { * reported by the server and surfaced as a toast). */ export default function MCollAddSheet({ open, collectionId, collectionName, lists, categories, onClose, onAdded, t }: MCollAddSheetProps) { - const { language } = useTranslation() - const placeLang = usePlaceLanguage() - const toast = useToast() - const [query, setQuery] = useState('') - const [results, setResults] = useState([]) - const [searching, setSearching] = useState(false) - const [picked, setPicked] = useState(null) - const [name, setName] = useState('') - const [address, setAddress] = useState('') - const [categoryId, setCategoryId] = useState(null) - const [status, setStatus] = useState('idea') - const [description, setDescription] = useState('') - const [saving, setSaving] = useState(false) - const [targetId, setTargetId] = useState(collectionId) - - useEffect(() => { - if (open) return - setQuery(''); setResults([]); setPicked(null); setName(''); setAddress(''); setCategoryId(null); setStatus('idea'); setDescription('') - setTargetId(null) - }, [open]) - - // Opened from a specific list → target is fixed; from "All Saved" (no active - // list) → default to the sole list if there is one, else pick it in-sheet. - // The lists may still be loading when the sheet opens, so the default is - // applied again once they arrive — without overruling a pick already made. - useEffect(() => { - if (!open) return - if (collectionId != null) { setTargetId(collectionId); return } - setTargetId(prev => (prev != null && lists.some(l => l.id === prev) ? prev : (lists.length === 1 ? lists[0].id : null))) - }, [open, collectionId, lists]) - - const search = async () => { - if (!query.trim() || searching) return - setSearching(true) - try { - const res = await mapsApi.search(query, placeLang) - setResults((res.places as MapsPlace[]) || []) - } catch (err) { - toast.error(getApiErrorMessage(err, t('places.mapsSearchError'))) - } finally { - setSearching(false) - } - } - - const pick = (r: MapsPlace) => { - setPicked(r) - setName(str(r.name) ?? '') - setAddress(str(r.address) ?? '') - setResults([]) - setQuery(str(r.name) ?? query) - } - - const save = async () => { - const cleanName = name.trim() - if (!cleanName || targetId == null || saving) return - setSaving(true) - try { - const res = await collectionsApi.savePlace({ - collection_id: targetId, - name: cleanName, - address: address.trim() || null, - lat: (picked && num(picked.lat)) ?? null, - lng: (picked && num(picked.lng)) ?? null, - google_place_id: (picked && str(picked.google_place_id)) ?? null, - google_ftid: (picked && str(picked.google_ftid)) ?? null, - osm_id: (picked && str(picked.osm_id)) ?? null, - website: (picked && str(picked.website)) ?? null, - phone: (picked && str(picked.phone)) ?? null, - category_id: categoryId, - description: description.trim() || null, - status, - force: true, - }) - if (res.duplicate) toast.info(t('collections.duplicateWarning')) - else { - toast.success(t('collections.addedToList', { name: lists.find(l => l.id === targetId)?.name ?? collectionName })) - onAdded() - } - onClose() - } catch (err) { - toast.error(getApiErrorMessage(err, t('common.error'))) - } finally { - setSaving(false) - } - } + const { + query, setQuery, results, setResults, searching, name, setName, address, setAddress, categoryId, setCategoryId, + status, setStatus, description, setDescription, saving, targetId, setTargetId, search, pick, save, + } = useAddPlaceToCollection({ variant: 'sheet', open, collectionId, collectionName, lists, t, onClose, onAdded }) return (