diff --git a/client/src/components/Planner/PlaceFormModal.test.tsx b/client/src/components/Planner/PlaceFormModal.test.tsx index ff5e1682f..f1e4abb29 100644 --- a/client/src/components/Planner/PlaceFormModal.test.tsx +++ b/client/src/components/Planner/PlaceFormModal.test.tsx @@ -299,6 +299,43 @@ describe('PlaceFormModal', () => { delete window.__addToast; }); + it('FE-PLANNER-PLACEFORM-021e: an OpenStreetMap suggestion keeps its own coordinates instead of searching for its label', async () => { + // The layer's second line is the name written on the building, not an + // address. Joining the two and searching for it asks a question nobody + // typed, and whatever came back first was taken as the place the user had + // already picked. The row carries coordinates, so the fallback uses those. + const user = userEvent.setup(); + let searched = 0; + server.use( + http.post('/api/maps/autocomplete', () => + HttpResponse.json({ + suggestions: [{ + placeId: 'node:9712313', + mainText: 'Tokio Hauptbahnhof', + secondaryText: '東京駅丸の内駅舎', + source: 'openstreetmap', + lat: 35.6811816, + lng: 139.76598265, + }], + source: 'trek-places', + }), + ), + http.get('/api/maps/details/:placeId', () => HttpResponse.json({ place: null, disabled: true })), + http.post('/api/maps/search', () => { + searched += 1; + return HttpResponse.json({ places: [{ name: 'Etwas ganz anderes', lat: '1', lng: '1' }], source: 'trek-places' }); + }), + ); + + render(); + await user.type(screen.getByPlaceholderText('Search places...'), 'Tokio'); + await user.click(await screen.findByText('Tokio Hauptbahnhof')); + + expect(await screen.findByDisplayValue('35.6811816')).toBeInTheDocument(); + expect(screen.getByDisplayValue('139.76598265')).toBeInTheDocument(); + expect(searched).toBe(0); + }); + it('FE-PLANNER-PLACEFORM-021c: suggestion click falls back when details is disabled (place: null)', async () => { const user = userEvent.setup(); server.use( @@ -375,6 +412,31 @@ describe('PlaceFormModal', () => { expect(await screen.findByText('TREK')).toBeInTheDocument(); }); + it('FE-PLANNER-PLACEFORM-022d: the typed-ahead list marks the OpenStreetMap rows as such', async () => { + // The keystroke path asks both indexes at once, so the name the response + // carries for the whole list ('trek-places') is true of the call and wrong + // for half the rows. Marking every suggestion TREK is how a list that did + // contain OpenStreetMap places read as if it never had. + const user = userEvent.setup(); + server.use( + http.post('/api/maps/autocomplete', () => + HttpResponse.json({ + suggestions: [ + { placeId: 'gers:abc', mainText: 'Tokyo Station Beer Stand', secondaryText: 'Chiyoda', source: 'trek-places' }, + { placeId: 'node:9712313', mainText: 'Tokio Hauptbahnhof', secondaryText: '東京駅', source: 'openstreetmap' }, + ], + source: 'trek-places', + }), + ), + ); + + render(); + await user.type(screen.getByPlaceholderText('Search places...'), 'Tokyo Station'); + + expect(await screen.findByText('TREK')).toBeInTheDocument(); + expect(await screen.findByText('OpenStreetMap')).toBeInTheDocument(); + }); + it('FE-PLANNER-PLACEFORM-022c: an interleaved result list marks each row with its own index', async () => { // The whole point of putting it on the row: with both indexes answering, // a label above the list could only name one of them. The index marks its diff --git a/client/src/components/Planner/PlaceFormModal.tsx b/client/src/components/Planner/PlaceFormModal.tsx index 8106b8bea..f6597cd1b 100644 --- a/client/src/components/Planner/PlaceFormModal.tsx +++ b/client/src/components/Planner/PlaceFormModal.tsx @@ -18,6 +18,7 @@ import { useTranslation } from '../../i18n' import CustomTimePicker from '../shared/CustomTimePicker' import { DEFAULT_FORM, isGoogleMapsUrl, mergeResult, type PlaceFormData, type ResultField } from './PlaceFormModal.helpers' import { getApiErrorMessage } from '../../utils/apiError' +import { sourceLabelFor } from '../../utils/placeSource' import { useLocationBias } from '../../hooks/useLocationBias' import { BookingCostsSection } from './BookingCostsSection' import type { BookingExpenseRequest } from './BookingCostsSection.types' @@ -56,36 +57,19 @@ interface PlaceFormModalProps { /** - * Which index a search result came from, as a short mark beside it. + * One row of the typed-ahead list, as the server sends it. * - * A place list can be two indexes interleaved, so the source belongs on the row - * rather than above the list: with the TREK index and OpenStreetMap answering - * together, "one of these came from somewhere" is not an answer anyone can use. - * - * The three names are proper nouns, so they are not translated, and that is also - * why there is no fourth: a source without a name people already know would need - * a string in 23 languages to say less than nothing. + * `source`, `lat` and `lng` are optional because not every index fills them: + * Google answers with neither, and the mark falls back to the name the whole + * list carries. */ -const SOURCE_LABELS: Record = { - 'trek-places': 'TREK', - openstreetmap: 'OpenStreetMap', - nominatim: 'OpenStreetMap', - google: 'Google', -} - -/** - * The label for one row. - * - * A place carries its own source when the index that produced it says so, which - * is what makes an interleaved list readable. Everything else falls back to what - * answered the call: Google never marks its places, and a merged list marks only - * the index side, so an unmarked row in one is OpenStreetMap by elimination. - */ -function sourceLabelFor(place: unknown, listSource: string): string | null { - const own = (place as { source?: unknown } | null)?.source - if (typeof own === 'string' && own) return SOURCE_LABELS[own] ?? null - if (listSource.includes('openstreetmap')) return SOURCE_LABELS.openstreetmap - return SOURCE_LABELS[listSource] ?? null +type Suggestion = { + placeId: string + mainText: string + secondaryText: string + source?: string + lat?: number + lng?: number } /** The mark itself. Quiet on purpose: it answers a question, it does not advertise. */ @@ -161,10 +145,10 @@ function usePlaceFormModal(props: PlaceFormModalProps) { const [pendingFiles, setPendingFiles] = useState([]) const fileRef = useRef(null) const searchInputRef = useRef(null) - const [acSuggestions, setAcSuggestions] = useState<{ placeId: string; mainText: string; secondaryText: string }[]>([]) - // Which index answered the last keystroke. One call is served by one source, - // so the whole list carries it; the search below is per place, because that - // list can be two indexes interleaved. + const [acSuggestions, setAcSuggestions] = useState([]) + // Which index answered the last keystroke, for the rows that do not say so + // themselves. Google and the OpenStreetMap fallback each answer from one + // place; the index path answers from two at once and marks every row. const [acSource, setAcSource] = useState('') const [acHighlight, setAcHighlight] = useState(-1) const acDebounceRef = useRef | null>(null) @@ -420,7 +404,7 @@ function usePlaceFormModal(props: PlaceFormModalProps) { setMapsSearch('') } - const handleSelectSuggestion = async (suggestion: { placeId: string; mainText: string; secondaryText: string }) => { + const handleSelectSuggestion = async (suggestion: Suggestion) => { // Read before the list is cleared: this is the rank the user saw. const acRank = acSuggestions.findIndex(s => s.placeId === suggestion.placeId) const acCount = acSuggestions.length @@ -448,6 +432,21 @@ function usePlaceFormModal(props: PlaceFormModalProps) { } catch (err) { console.error('Failed to fetch place details:', err) } + if (!place && suggestion.source === 'openstreetmap' && suggestion.lat != null && suggestion.lng != null) { + // The layer's rows carry no address; their second line is the name + // written on the building. Searching for "Tokio Hauptbahnhof, 東京駅" + // is not a question anybody asked, and its first answer would be + // whatever the index made of it — a different place, chosen silently. + // The suggestion already knows where it is, so use that. + place = { + name: suggestion.mainText, + address: '', + lat: suggestion.lat, + lng: suggestion.lng, + osm_id: suggestion.placeId, + source: 'openstreetmap', + } + } if (!place) { const query = [suggestion.mainText, suggestion.secondaryText].filter(Boolean).join(', ') const search = await mapsApi.search(query, language, locationBiasPoint) diff --git a/client/src/mobile/screens/trip/sheets/PlPlaceSearch.tsx b/client/src/mobile/screens/trip/sheets/PlPlaceSearch.tsx index e6ead9394..ec1b26f04 100644 --- a/client/src/mobile/screens/trip/sheets/PlPlaceSearch.tsx +++ b/client/src/mobile/screens/trip/sheets/PlPlaceSearch.tsx @@ -1,6 +1,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { Loader2, Search } from 'lucide-react' import { mapsApi } from '../../../../api/client' +import { sourceLabelFor } from '../../../../utils/placeSource' import { recordPlacePick } from '../../../../api/placeShadow' import { PlacesSession } from '../../../../utils/placesSession' import { isGoogleMapsUrl } from '../../../../components/Planner/PlaceFormModal.helpers' @@ -26,10 +27,24 @@ interface Suggestion { placeId: string mainText: string secondaryText: string + /** Which of the two indexes this row came from, when the list is both. */ + source?: string + lat?: number + lng?: number } type MapsPlace = Record +/** The same mark the desktop form shows, in the sheet's own tokens. */ +function SourceMark({ label }: { label: string | null }) { + if (!label) return null + return ( + + {label} + + ) +} + interface PlPlaceSearchProps { planner: TripPlanner /** Search bias derived from the trip's existing places (trip centre). */ @@ -76,6 +91,8 @@ export default function PlPlaceSearch({ planner, locationBias, onPick, onResolvi // has to read the values that belonged to that list. Mirrors PlaceFormModal. const searchMetaRef = useRef<{ query: string; source: string } | null>(null) const acMetaRef = useRef<{ query: string; source: string } | null>(null) + // The name the whole list carries, for rows that do not name their own index. + const [acSource, setAcSource] = useState('') const setResolving = useCallback( (v: boolean) => { @@ -93,6 +110,7 @@ export default function PlPlaceSearch({ planner, locationBias, onPick, onResolvi try { const result = await mapsApi.autocomplete(input, language, locationBias, controller.signal, placesSessionRef.current.current()) acMetaRef.current = { query: input, source: result.source || 'unknown' } + setAcSource(result.source || '') setSuggestions(result.suggestions || []) } catch (err: unknown) { // Superseded request — axios rejects an aborted call with CanceledError. @@ -211,6 +229,20 @@ export default function PlPlaceSearch({ planner, locationBias, onPick, onResolvi } catch { // fall through to text search } + if (!place && suggestion.source === 'openstreetmap' && suggestion.lat != null && suggestion.lng != null) { + // The layer's second line is the local name, not an address, so joining + // the two makes a query nobody typed — and its first answer would be + // silently taken as the place the user picked. The row already knows + // where it is. + place = { + name: suggestion.mainText, + address: '', + lat: suggestion.lat, + lng: suggestion.lng, + osm_id: suggestion.placeId, + source: 'openstreetmap', + } + } if (!place) { const fullQuery = [suggestion.mainText, suggestion.secondaryText].filter(Boolean).join(', ') const search = await mapsApi.search(fullQuery, language, pointFromBox(locationBias)) @@ -269,10 +301,15 @@ export default function PlPlaceSearch({ planner, locationBias, onPick, onResolvi onClick={() => handleSelectSuggestion(s)} className="block w-full border-t border-[color:var(--m-rowbr)] px-[13px] py-[10px] text-left first:border-t-0" > -
{s.mainText}
- {s.secondaryText && ( -
{s.secondaryText}
- )} +
+
+
{s.mainText}
+ {s.secondaryText && ( +
{s.secondaryText}
+ )} +
+ +
))} diff --git a/client/src/utils/placeSource.ts b/client/src/utils/placeSource.ts new file mode 100644 index 000000000..0d651cf5e --- /dev/null +++ b/client/src/utils/placeSource.ts @@ -0,0 +1,39 @@ +/** + * Which index a place came from, as a short mark beside it. + * + * A result list can be two indexes interleaved, so the source belongs on the + * row rather than above the list: with the TREK index and OpenStreetMap + * answering together, "one of these came from somewhere" is not an answer + * anyone can use. + * + * Here rather than beside one screen because both the desktop form and the + * mobile sheet show the same lists, and a mark that says TREK on one and + * nothing on the other is worse than either alone. + */ + +/** + * The three names are proper nouns, so they are not translated, and that is + * also why there is no fourth: a source without a name people already know + * would need a string in 23 languages to say less than nothing. + */ +export const SOURCE_LABELS: Record = { + 'trek-places': 'TREK', + openstreetmap: 'OpenStreetMap', + nominatim: 'OpenStreetMap', + google: 'Google', +} + +/** + * The label for one row. + * + * A place carries its own source when the index that produced it says so, which + * is what makes an interleaved list readable. Everything else falls back to what + * answered the call: Google never marks its places, and a merged list marks only + * the index side, so an unmarked row in one is OpenStreetMap by elimination. + */ +export function sourceLabelFor(place: unknown, listSource: string): string | null { + const own = (place as { source?: unknown } | null)?.source + if (typeof own === 'string' && own) return SOURCE_LABELS[own] ?? null + if (listSource.includes('openstreetmap')) return SOURCE_LABELS.openstreetmap + return SOURCE_LABELS[listSource] ?? null +} diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index a5eaa085a..016f6c40c 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -2021,7 +2021,7 @@ export class MapsService { lang?: string, locationBias?: { low: { lat: number; lng: number }; high: { lat: number; lng: number } }, sessionToken?: string, - ): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> { + ): Promise { const { key: apiKey, source: keySource } = this.resolveMapsKey(userId); // This is the path that mattered most. Nominatim's usage policy names @@ -2071,11 +2071,25 @@ export class MapsService { // place is called on the spot, which is more use under a // translated label than an empty line. secondaryText: p.local_name && p.local_name !== p.name ? p.local_name : '', + // Per row, because this list is two indexes interleaved. + // The name above the list says `trek-places`, which is true + // of the call and false of half the rows in it — the layer + // is OpenStreetMap, and a reader deciding whether to trust + // a suggestion is asking exactly that. + source: 'openstreetmap', + // Both indexes hand these over with the row. Carried rather + // than dropped so picking a suggestion has something to fall + // back on when the details lookup cannot answer. + lat: p.lat, + lng: p.lng, } : { placeId: `gers:${p.gers}`, mainText: p.name, secondaryText: [p.address?.locality, p.address?.country].filter(Boolean).join(', '), + source: 'trek-places', + lat: p.lat, + lng: p.lng, }, ), source: 'trek-places', @@ -2143,7 +2157,7 @@ export class MapsService { private async autocompleteNominatim( input: string, lang?: string, - ): Promise<{ suggestions: { placeId: string; mainText: string; secondaryText: string }[]; source: string }> { + ): Promise { try { const places = await this.searchNominatim(input, lang); const suggestions = places diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts index 5f4fcd71c..b0705216d 100644 --- a/server/tests/unit/nest/maps.autocomplete.test.ts +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -196,6 +196,19 @@ describe('MapsService.autocompletePlaces', () => { expect(second.suggestions[0].secondaryText).toBe(''); }); + it('MAPS-AUTO-010: each row says which index it came from, because the list is two', async () => { + mockSearch.mockResolvedValue([hit(), osmHit()]); + + const { suggestions, source } = await make().autocompletePlaces(1, INPUT); + + // The name above the list describes the call, and the call asked both. Only + // the row can say which of the two answered it — without that the reader is + // told a place from OpenStreetMap came out of the TREK index, which is the + // one thing the mark beside a suggestion exists to answer. + expect(source).toBe('trek-places'); + expect(suggestions.map(s => s.source)).toEqual(['trek-places', 'openstreetmap']); + }); + it('MAPS-AUTO-007: the keystroke never leaves for the index while the admin has it off', async () => { mockSearch.mockResolvedValue([hit()]); diff --git a/shared/src/maps/maps.schema.ts b/shared/src/maps/maps.schema.ts index f79b8ff9a..729343eac 100644 --- a/shared/src/maps/maps.schema.ts +++ b/shared/src/maps/maps.schema.ts @@ -68,6 +68,26 @@ export const mapsAutocompleteSuggestionSchema = z.object({ placeId: z.string(), mainText: z.string(), secondaryText: z.string(), + /** + * Which index this one row came from, when the answer is more than one index + * interleaved. The list-level `source` cannot say it: the keystroke path asks + * the TREK index and the OpenStreetMap layer together, so a single name above + * the list marks half the rows wrong. + * + * Optional because Google and the Nominatim fallback each answer from one + * place, and a row that names no source falls back to the list's. + */ + source: z.string().optional(), + /** + * Where the place is, when the index that answered already said so. + * + * The OpenStreetMap layer returns coordinates with every row, and throwing + * them away here cost the client a second round trip on every pick — and, + * when that round trip failed, a text search built out of a name and its + * local spelling, which is not a query anybody would type. + */ + lat: z.number().optional(), + lng: z.number().optional(), }); export const mapsAutocompleteResultSchema = z.object({ suggestions: z.array(mapsAutocompleteSuggestionSchema),