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 (