diff --git a/client/src/api/client.ts b/client/src/api/client.ts index d11fa0c2..9529f90f 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -347,8 +347,8 @@ export const journeyApi = { export const mapsApi = { search: (query: string, lang?: string) => apiClient.post(`/maps/search?lang=${lang || 'en'}`, { query }).then(r => r.data), - autocomplete: (input: string, lang?: string, locationBias?: { low: { lat: number; lng: number }; high: { lat: number; lng: number } }) => - apiClient.post('/maps/autocomplete', { input, lang, locationBias }).then(r => r.data), + autocomplete: (input: string, lang?: string, locationBias?: { low: { lat: number; lng: number }; high: { lat: number; lng: number } }, signal?: AbortSignal) => + apiClient.post('/maps/autocomplete', { input, lang, locationBias }, { signal }).then(r => r.data), details: (placeId: string, lang?: string) => apiClient.get(`/maps/details/${encodeURIComponent(placeId)}`, { params: { lang } }).then(r => r.data), placePhoto: (placeId: string, lat?: number, lng?: number, name?: string) => apiClient.get(`/maps/place-photo/${encodeURIComponent(placeId)}`, { params: { lat, lng, name } }).then(r => r.data), reverse: (lat: number, lng: number, lang?: string) => apiClient.get('/maps/reverse', { params: { lat, lng, lang } }).then(r => r.data), diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 9f365696..1b1588c9 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef, useMemo } from 'react' +import { useState, useEffect, useRef, useMemo, useCallback } from 'react' import Modal from '../shared/Modal' import CustomSelect from '../shared/CustomSelect' import { mapsApi } from '../../api/client' @@ -87,7 +87,7 @@ export default function PlaceFormModal({ const [acSuggestions, setAcSuggestions] = useState<{ placeId: string; mainText: string; secondaryText: string }[]>([]) const [acHighlight, setAcHighlight] = useState(-1) const acDebounceRef = useRef | null>(null) - const [acTrigger, setAcTrigger] = useState(0) + const acAbortRef = useRef(null) const toast = useToast() const { t, language } = useTranslation() const { hasMapsKey } = useAuthStore() @@ -151,7 +151,29 @@ export default function PlaceFormModal({ return { low: { lat: minLat, lng: minLng }, high: { lat: maxLat, lng: maxLng } } }, [places]) - // Debounced autocomplete + // Autocomplete fetch — aborts any in-flight request before starting a new one + const fetchSuggestions = useCallback(async (query: string) => { + if (query.length < 2 || isGoogleMapsUrl(query)) { + setAcSuggestions([]) + setAcHighlight(-1) + return + } + acAbortRef.current?.abort() + const controller = new AbortController() + acAbortRef.current = controller + try { + const result = await mapsApi.autocomplete(query, language, locationBias, controller.signal) + setAcSuggestions(result.suggestions || []) + setAcHighlight(-1) + } catch (err: unknown) { + if (err instanceof Error && err.name === 'AbortError') return + if (err instanceof Error && err.name === 'CanceledError') return // axios abort + console.error('Autocomplete failed:', err) + setAcSuggestions([]) + } + }, [language, locationBias]) + + // Debounce effect — only watches mapsSearch useEffect(() => { if (acDebounceRef.current) clearTimeout(acDebounceRef.current) @@ -162,21 +184,12 @@ export default function PlaceFormModal({ return } - acDebounceRef.current = setTimeout(async () => { - try { - const result = await mapsApi.autocomplete(trimmed, language, locationBias) - setAcSuggestions(result.suggestions || []) - setAcHighlight(-1) - } catch (err) { - console.error('Autocomplete failed:', err) - setAcSuggestions([]) - } - }, 300) + acDebounceRef.current = setTimeout(() => fetchSuggestions(trimmed), 300) return () => { if (acDebounceRef.current) clearTimeout(acDebounceRef.current) } - }, [mapsSearch, language, locationBias, acTrigger]) + }, [mapsSearch, fetchSuggestions]) const handleChange = (field, value) => { setForm(prev => ({ ...prev, [field]: value })) @@ -366,7 +379,7 @@ export default function PlaceFormModal({ onBlur={() => setTimeout(() => setAcSuggestions([]), 150)} onFocus={() => { if (mapsSearch.trim().length >= 2 && acSuggestions.length === 0 && mapsResults.length === 0) { - setAcTrigger(prev => prev + 1) + fetchSuggestions(mapsSearch.trim()) } }} placeholder={t('places.mapsSearchPlaceholder')} @@ -436,8 +449,8 @@ export default function PlaceFormModal({ className="form-input" /> {isSearchingMaps && ( -
- +
+
)}
diff --git a/client/tests/integration/api/client.test.ts b/client/tests/integration/api/client.test.ts index ff588387..11d6a39f 100644 --- a/client/tests/integration/api/client.test.ts +++ b/client/tests/integration/api/client.test.ts @@ -954,4 +954,21 @@ describe('mapsApi', () => { await expect(mapsApi.autocomplete('test')).rejects.toThrow(); }); + + it('FE-MAPS-004: mapsApi.autocomplete rejects when AbortSignal is aborted', async () => { + const controller = new AbortController(); + + server.use( + http.post('/api/maps/autocomplete', async () => { + // Never resolves — request will be aborted + await new Promise(() => {}); + return HttpResponse.json({ suggestions: [] }); + }) + ); + + const promise = mapsApi.autocomplete('Paris', undefined, undefined, controller.signal); + controller.abort(); + + await expect(promise).rejects.toThrow(); + }); }); diff --git a/server/src/routes/maps.ts b/server/src/routes/maps.ts index 6b010f44..57867095 100644 --- a/server/src/routes/maps.ts +++ b/server/src/routes/maps.ts @@ -39,6 +39,10 @@ router.post('/autocomplete', authenticate, async (req: Request, res: Response) = return res.status(400).json({ error: 'Input is required' }); } + if (input.length > 200) { + return res.status(400).json({ error: 'Input too long (max 200 chars)' }); + } + if (locationBias) { const { low, high } = locationBias; if (!low || !high diff --git a/server/src/services/mapsService.ts b/server/src/services/mapsService.ts index dc30ce52..ca886cea 100644 --- a/server/src/services/mapsService.ts +++ b/server/src/services/mapsService.ts @@ -406,14 +406,17 @@ async function autocompleteNominatim( ): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> { try { const places = await searchNominatim(input, lang); - const suggestions = places.slice(0, 5).map((p) => { - const parts = (p.address || '').split(',').map((s) => s.trim()); - return { - placeId: p.osm_id || '', - mainText: p.name || parts[0] || '', - secondaryText: parts.slice(1).join(', '), - }; - }); + const suggestions = places + .filter((p) => p.osm_id && p.osm_id.includes(':') && p.osm_id.split(':')[1] !== '') + .slice(0, 5) + .map((p) => { + const parts = (p.address || '').split(',').map((s) => s.trim()); + return { + placeId: p.osm_id, + mainText: p.name || parts[0] || '', + secondaryText: parts.slice(1).join(', '), + }; + }); return { suggestions, source: 'nominatim' }; } catch (err) { console.error('Nominatim autocomplete failed:', err); @@ -427,18 +430,21 @@ export async function getPlaceDetails(userId: number, placeId: string, lang?: st // OSM details: placeId is "node:123456" or "way:123456" etc. if (placeId.includes(':')) { const [osmType, osmId] = placeId.split(':'); - const [element, nominatim] = await Promise.all([ - fetchOverpassDetails(osmType, osmId), - lookupNominatim(osmType, osmId, lang), - ]); + const element = await fetchOverpassDetails(osmType, osmId); const details = buildOsmDetails(element?.tags || {}, osmType, osmId); + + // Fetch Nominatim only when Overpass lacks coordinates or address + const d = details as Record; + const needsNominatim = !d.lat || !d.lng || !d.address; + const nominatim = needsNominatim ? await lookupNominatim(osmType, osmId, lang) : null; + return { place: { ...details, - name: nominatim?.name || element?.tags?.name || '', - address: nominatim?.address || '', - lat: nominatim?.lat ?? null, - lng: nominatim?.lng ?? null, + name: (d.name as string) || nominatim?.name || element?.tags?.name || '', + address: (d.address as string) || nominatim?.address || '', + lat: d.lat ?? nominatim?.lat ?? null, + lng: d.lng ?? nominatim?.lng ?? null, osm_id: placeId, }, }; diff --git a/server/tests/integration/maps.test.ts b/server/tests/integration/maps.test.ts index 6e7c4223..bad3fa85 100644 --- a/server/tests/integration/maps.test.ts +++ b/server/tests/integration/maps.test.ts @@ -383,4 +383,16 @@ describe('Maps autocomplete', () => { expect(res.status).toBe(500); }); + + it('MAPS-017 — POST /maps/autocomplete with input > 200 chars returns 400', async () => { + const { user } = createUser(testDb); + + const res = await request(app) + .post('/api/maps/autocomplete') + .set('Cookie', authCookie(user.id)) + .send({ input: 'a'.repeat(201) }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/too long/i); + }); }); diff --git a/server/tests/unit/services/mapsService.test.ts b/server/tests/unit/services/mapsService.test.ts index abf3f458..4399651a 100644 --- a/server/tests/unit/services/mapsService.test.ts +++ b/server/tests/unit/services/mapsService.test.ts @@ -908,6 +908,21 @@ describe('autocompletePlaces (fetch stubbed)', () => { expect(result.suggestions[0].mainText).toBe('Big Ben'); expect(result.suggestions[0].secondaryText).toBe('Westminster, London, UK'); }); + + it('MAPS-093: Nominatim fallback filters out results with empty osm_id', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + json: async () => [ + { osm_type: 'node', osm_id: '1', lat: '48.8', lon: '2.3', display_name: 'Paris, France', name: 'Paris' }, + { osm_type: 'node', osm_id: '', lat: '51.5', lon: '-0.1', display_name: 'London, UK', name: 'London' }, + { osm_type: 'way', osm_id: '3', lat: '52.5', lon: '13.4', display_name: 'Berlin, Germany', name: 'Berlin' }, + ], + })); + const { autocompletePlaces } = await import('../../../src/services/mapsService'); + const result = await autocompletePlaces(999, 'test'); + expect(result.suggestions).toHaveLength(2); + expect(result.suggestions.map((s) => s.placeId)).toEqual(['node:1', 'way:3']); + }); }); // ── getPlaceDetails (fetch stubbed) ───────────────────────────────────────── @@ -1023,6 +1038,37 @@ describe('getPlaceDetails (fetch stubbed)', () => { expect(review.photo).toBeNull(); }); + it('MAPS-040c: OSM path enriches name/address/coords from Nominatim (serial fetch)', async () => { + const fetchMock = vi.fn() + // First call: Overpass (returns element with tags but no coords) + .mockResolvedValueOnce({ + ok: true, + json: async () => ({ elements: [{ tags: { website: 'https://example.com' } }] }), + }) + // Second call: Nominatim /lookup + .mockResolvedValueOnce({ + ok: true, + json: async () => [ + { osm_type: 'way', osm_id: '5', lat: '48.85', lon: '2.29', display_name: 'Eiffel Tower, Paris, France', name: 'Eiffel Tower' }, + ], + }); + vi.stubGlobal('fetch', fetchMock); + const { getPlaceDetails } = await import('../../../src/services/mapsService'); + const result = await getPlaceDetails(1, 'way:5'); + const place = result.place as any; + expect(place.name).toBe('Eiffel Tower'); + expect(place.address).toBe('Eiffel Tower, Paris, France'); + expect(place.lat).toBeCloseTo(48.85); + expect(place.lng).toBeCloseTo(2.29); + expect(place.source).toBe('openstreetmap'); + // Overpass first, then Nominatim — two total fetch calls + expect(fetchMock).toHaveBeenCalledTimes(2); + const overpassUrl = fetchMock.mock.calls[0][0] as string; + const nominatimUrl = fetchMock.mock.calls[1][0] as string; + expect(overpassUrl).toContain('overpass'); + expect(nominatimUrl).toContain('nominatim'); + }); + it('MAPS-041e: open_now is null when regularOpeningHours.openNow is undefined', async () => { mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' }); vi.stubGlobal('fetch', vi.fn().mockResolvedValue({