mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-30 18:46:00 +00:00
fix(search-autocomplete): address PR #542 review issues
- Fix race condition: AbortController cancels in-flight autocomplete requests on each keystroke; stale responses no longer overwrite fresh ones - Remove acTrigger state hack; onFocus calls fetchSuggestions directly - Cap autocomplete input at 200 chars server-side (400 on violation) - Filter Nominatim suggestions with empty osm_id segments - Revert getPlaceDetails OSM branch from unconditional parallel fetch to conditional serial: Nominatim called only when Overpass lacks coords/address - Wire places.loadingDetails i18n key to Loader2 spinner via aria-label/role - Add tests: MAPS-017, MAPS-040c, MAPS-093, FE-MAPS-004
This commit is contained in:
@@ -347,8 +347,8 @@ export const journeyApi = {
|
|||||||
|
|
||||||
export const mapsApi = {
|
export const mapsApi = {
|
||||||
search: (query: string, lang?: string) => apiClient.post(`/maps/search?lang=${lang || 'en'}`, { query }).then(r => r.data),
|
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 } }) =>
|
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 }).then(r => r.data),
|
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),
|
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),
|
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),
|
reverse: (lat: number, lng: number, lang?: string) => apiClient.get('/maps/reverse', { params: { lat, lng, lang } }).then(r => r.data),
|
||||||
|
|||||||
@@ -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 Modal from '../shared/Modal'
|
||||||
import CustomSelect from '../shared/CustomSelect'
|
import CustomSelect from '../shared/CustomSelect'
|
||||||
import { mapsApi } from '../../api/client'
|
import { mapsApi } from '../../api/client'
|
||||||
@@ -87,7 +87,7 @@ export default function PlaceFormModal({
|
|||||||
const [acSuggestions, setAcSuggestions] = useState<{ placeId: string; mainText: string; secondaryText: string }[]>([])
|
const [acSuggestions, setAcSuggestions] = useState<{ placeId: string; mainText: string; secondaryText: string }[]>([])
|
||||||
const [acHighlight, setAcHighlight] = useState(-1)
|
const [acHighlight, setAcHighlight] = useState(-1)
|
||||||
const acDebounceRef = useRef<ReturnType<typeof setTimeout> | null>(null)
|
const acDebounceRef = useRef<ReturnType<typeof setTimeout> | null>(null)
|
||||||
const [acTrigger, setAcTrigger] = useState(0)
|
const acAbortRef = useRef<AbortController | null>(null)
|
||||||
const toast = useToast()
|
const toast = useToast()
|
||||||
const { t, language } = useTranslation()
|
const { t, language } = useTranslation()
|
||||||
const { hasMapsKey } = useAuthStore()
|
const { hasMapsKey } = useAuthStore()
|
||||||
@@ -151,7 +151,29 @@ export default function PlaceFormModal({
|
|||||||
return { low: { lat: minLat, lng: minLng }, high: { lat: maxLat, lng: maxLng } }
|
return { low: { lat: minLat, lng: minLng }, high: { lat: maxLat, lng: maxLng } }
|
||||||
}, [places])
|
}, [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(() => {
|
useEffect(() => {
|
||||||
if (acDebounceRef.current) clearTimeout(acDebounceRef.current)
|
if (acDebounceRef.current) clearTimeout(acDebounceRef.current)
|
||||||
|
|
||||||
@@ -162,21 +184,12 @@ export default function PlaceFormModal({
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
acDebounceRef.current = setTimeout(async () => {
|
acDebounceRef.current = setTimeout(() => fetchSuggestions(trimmed), 300)
|
||||||
try {
|
|
||||||
const result = await mapsApi.autocomplete(trimmed, language, locationBias)
|
|
||||||
setAcSuggestions(result.suggestions || [])
|
|
||||||
setAcHighlight(-1)
|
|
||||||
} catch (err) {
|
|
||||||
console.error('Autocomplete failed:', err)
|
|
||||||
setAcSuggestions([])
|
|
||||||
}
|
|
||||||
}, 300)
|
|
||||||
|
|
||||||
return () => {
|
return () => {
|
||||||
if (acDebounceRef.current) clearTimeout(acDebounceRef.current)
|
if (acDebounceRef.current) clearTimeout(acDebounceRef.current)
|
||||||
}
|
}
|
||||||
}, [mapsSearch, language, locationBias, acTrigger])
|
}, [mapsSearch, fetchSuggestions])
|
||||||
|
|
||||||
const handleChange = (field, value) => {
|
const handleChange = (field, value) => {
|
||||||
setForm(prev => ({ ...prev, [field]: value }))
|
setForm(prev => ({ ...prev, [field]: value }))
|
||||||
@@ -366,7 +379,7 @@ export default function PlaceFormModal({
|
|||||||
onBlur={() => setTimeout(() => setAcSuggestions([]), 150)}
|
onBlur={() => setTimeout(() => setAcSuggestions([]), 150)}
|
||||||
onFocus={() => {
|
onFocus={() => {
|
||||||
if (mapsSearch.trim().length >= 2 && acSuggestions.length === 0 && mapsResults.length === 0) {
|
if (mapsSearch.trim().length >= 2 && acSuggestions.length === 0 && mapsResults.length === 0) {
|
||||||
setAcTrigger(prev => prev + 1)
|
fetchSuggestions(mapsSearch.trim())
|
||||||
}
|
}
|
||||||
}}
|
}}
|
||||||
placeholder={t('places.mapsSearchPlaceholder')}
|
placeholder={t('places.mapsSearchPlaceholder')}
|
||||||
@@ -436,8 +449,8 @@ export default function PlaceFormModal({
|
|||||||
className="form-input"
|
className="form-input"
|
||||||
/>
|
/>
|
||||||
{isSearchingMaps && (
|
{isSearchingMaps && (
|
||||||
<div className="absolute right-2.5 top-0 bottom-0 flex items-center">
|
<div className="absolute right-2.5 top-0 bottom-0 flex items-center" role="status" aria-label={t('places.loadingDetails')}>
|
||||||
<Loader2 className="w-4 h-4 animate-spin text-slate-400" />
|
<Loader2 className="w-4 h-4 animate-spin text-slate-400" aria-hidden="true" />
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -954,4 +954,21 @@ describe('mapsApi', () => {
|
|||||||
|
|
||||||
await expect(mapsApi.autocomplete('test')).rejects.toThrow();
|
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();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -39,6 +39,10 @@ router.post('/autocomplete', authenticate, async (req: Request, res: Response) =
|
|||||||
return res.status(400).json({ error: 'Input is required' });
|
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) {
|
if (locationBias) {
|
||||||
const { low, high } = locationBias;
|
const { low, high } = locationBias;
|
||||||
if (!low || !high
|
if (!low || !high
|
||||||
|
|||||||
@@ -406,14 +406,17 @@ async function autocompleteNominatim(
|
|||||||
): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> {
|
): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> {
|
||||||
try {
|
try {
|
||||||
const places = await searchNominatim(input, lang);
|
const places = await searchNominatim(input, lang);
|
||||||
const suggestions = places.slice(0, 5).map((p) => {
|
const suggestions = places
|
||||||
const parts = (p.address || '').split(',').map((s) => s.trim());
|
.filter((p) => p.osm_id && p.osm_id.includes(':') && p.osm_id.split(':')[1] !== '')
|
||||||
return {
|
.slice(0, 5)
|
||||||
placeId: p.osm_id || '',
|
.map((p) => {
|
||||||
mainText: p.name || parts[0] || '',
|
const parts = (p.address || '').split(',').map((s) => s.trim());
|
||||||
secondaryText: parts.slice(1).join(', '),
|
return {
|
||||||
};
|
placeId: p.osm_id,
|
||||||
});
|
mainText: p.name || parts[0] || '',
|
||||||
|
secondaryText: parts.slice(1).join(', '),
|
||||||
|
};
|
||||||
|
});
|
||||||
return { suggestions, source: 'nominatim' };
|
return { suggestions, source: 'nominatim' };
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error('Nominatim autocomplete failed:', 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.
|
// OSM details: placeId is "node:123456" or "way:123456" etc.
|
||||||
if (placeId.includes(':')) {
|
if (placeId.includes(':')) {
|
||||||
const [osmType, osmId] = placeId.split(':');
|
const [osmType, osmId] = placeId.split(':');
|
||||||
const [element, nominatim] = await Promise.all([
|
const element = await fetchOverpassDetails(osmType, osmId);
|
||||||
fetchOverpassDetails(osmType, osmId),
|
|
||||||
lookupNominatim(osmType, osmId, lang),
|
|
||||||
]);
|
|
||||||
const details = buildOsmDetails(element?.tags || {}, osmType, osmId);
|
const details = buildOsmDetails(element?.tags || {}, osmType, osmId);
|
||||||
|
|
||||||
|
// Fetch Nominatim only when Overpass lacks coordinates or address
|
||||||
|
const d = details as Record<string, unknown>;
|
||||||
|
const needsNominatim = !d.lat || !d.lng || !d.address;
|
||||||
|
const nominatim = needsNominatim ? await lookupNominatim(osmType, osmId, lang) : null;
|
||||||
|
|
||||||
return {
|
return {
|
||||||
place: {
|
place: {
|
||||||
...details,
|
...details,
|
||||||
name: nominatim?.name || element?.tags?.name || '',
|
name: (d.name as string) || nominatim?.name || element?.tags?.name || '',
|
||||||
address: nominatim?.address || '',
|
address: (d.address as string) || nominatim?.address || '',
|
||||||
lat: nominatim?.lat ?? null,
|
lat: d.lat ?? nominatim?.lat ?? null,
|
||||||
lng: nominatim?.lng ?? null,
|
lng: d.lng ?? nominatim?.lng ?? null,
|
||||||
osm_id: placeId,
|
osm_id: placeId,
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -383,4 +383,16 @@ describe('Maps autocomplete', () => {
|
|||||||
|
|
||||||
expect(res.status).toBe(500);
|
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);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -908,6 +908,21 @@ describe('autocompletePlaces (fetch stubbed)', () => {
|
|||||||
expect(result.suggestions[0].mainText).toBe('Big Ben');
|
expect(result.suggestions[0].mainText).toBe('Big Ben');
|
||||||
expect(result.suggestions[0].secondaryText).toBe('Westminster, London, UK');
|
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) ─────────────────────────────────────────
|
// ── getPlaceDetails (fetch stubbed) ─────────────────────────────────────────
|
||||||
@@ -1023,6 +1038,37 @@ describe('getPlaceDetails (fetch stubbed)', () => {
|
|||||||
expect(review.photo).toBeNull();
|
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 () => {
|
it('MAPS-041e: open_now is null when regularOpeningHours.openNow is undefined', async () => {
|
||||||
mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' });
|
mockDbGet.mockReturnValueOnce({ maps_api_key: 'gkey' });
|
||||||
vi.stubGlobal('fetch', vi.fn().mockResolvedValue({
|
vi.stubGlobal('fetch', vi.fn().mockResolvedValue({
|
||||||
|
|||||||
Reference in New Issue
Block a user