From ed5769fc89d607041a157be8e9b82dbc62482d5e Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 7 Sep 2026 11:49:39 +0200 Subject: [PATCH] fix(maps): name the index as the source, and stop it being asked twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attribution first, because this branch argues the point itself: the card names the sources with the comment that naming them is a licence obligation rather than decoration, and the copy beside it promises that every field says where it came from. The POI path then stamped Overture rows as OpenStreetMap. Overture is not OpenStreetMap — it carries OSM among other sources under other licences — so that is a wrong attribution rather than a rounded one. The wire contract keeps `source` an open string and nothing on the client switches on it, so saying `trek-places` costs nothing. The MCP tool description no longer calls itself an OpenStreetMap tool either, and says which source answered. Adding one place cost two round trips to the index. The details lookup fetches the place and its answer carries the description; toPlaceRecord dropped that field, so the enrichment fetched the same place again a moment later to read it. It is carried through now and read from what the caller already has, with the second lookup left as the fallback for when there is nothing to read. That description's source URL becomes an href on the client and was taken on `typeof === 'string'` alone. It now goes through the same allow-list a place's website goes through — the file already does this for menu_url, with the comment explaining why — so anything but http(s) loses the link and keeps the text. The instance token was hashed from raw APP_URL, which the shipped compose file leaves commented out. Every default install therefore sent the same token, and a token every caller shares is not a caller: one instance running a large import would rate-limit all the others, who would fall back to Nominatim with nothing to point at. getAppUrl() falls through to ALLOWED_ORIGINS and then to the port, so unconfigured instances still differ. Two client-side leaks. A pick made out of the offline cache was recorded into the shadow corpus, where it moved the two figures the corpus exists to measure — and offline is a setting, so the network is usually reachable and the row landed. And the prefetch never re-checked the session after its request came back: a logout in that window repoints the handle at the anonymous database, so one person's trip area was written where the next person's offline search reads. The cache path also ignored the abort signal, so an answer for a keystroke two keystrokes old could overwrite a newer suggestion list. It rejects the way both surfaces already know to swallow. Left alone, and said in a comment rather than hidden: the index cannot localise. The Overpass path picks name: and falls back to int_name; the service has no language parameter at all, so a German reader exploring Tokyo gets the Japanese primary names on this path. --- client/src/api/client.ts | 25 ++++++++- client/src/api/placeShadow.test.ts | 17 ++++++ client/src/api/placeShadow.ts | 9 ++++ client/src/sync/placePrefetcher.test.ts | 18 +++++++ client/src/sync/placePrefetcher.ts | 8 +++ server/src/nest/maps/maps.helpers.ts | 8 ++- server/src/nest/maps/maps.mcp.ts | 2 +- server/src/nest/maps/maps.service.ts | 27 +++++++--- server/src/nest/maps/trek-places.client.ts | 16 +++++- .../place-enrichment.service.ts | 25 +++++++-- .../unit/mcp/tools-tags-maps-weather.test.ts | 6 ++- server/tests/unit/nest/maps.pois.test.ts | 15 +++--- .../nest/place-enrichment.description.test.ts | 50 +++++++++++++++++ .../unit/nest/trek-places.client.test.ts | 54 ++++++++++++++++++- 14 files changed, 254 insertions(+), 26 deletions(-) diff --git a/client/src/api/client.ts b/client/src/api/client.ts index d73f6d05c..5f23bed9a 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -1060,6 +1060,19 @@ export const memoriesApi = { * by a query, and a miss has to stay a miss — a place the cache does not hold * must fail the way it did before, not answer with somebody else's record. */ +/** + * The rejection both search surfaces already know to swallow. + * + * They filter on the axios cancel code and on the DOMException name, so an + * abort from the cache path has to look like one or it surfaces as a toast. + */ +function abortedError(): Error & { code: string } { + const err = new Error('canceled') as Error & { code: string } + err.name = 'CanceledError' + err.code = 'ERR_CANCELED' + return err +} + async function withCachedPlace( placeId: string, online: () => Promise<{ place: Record | null }>, @@ -1091,10 +1104,19 @@ async function withCachedPlaces( query: string, shape: (places: Record[]) => T, online: () => Promise, + signal?: AbortSignal, ): Promise { const fromCache = async (): Promise[]> => { const { searchCachedPlaces, cachedToPlaceRecord } = await import('../sync/placePrefetcher') - return (await searchCachedPlaces(query)).map(cachedToPlaceRecord) + const found = (await searchCachedPlaces(query)).map(cachedToPlaceRecord) + // The keystroke this answers may already be two keystrokes old. Both search + // surfaces order their suggestions by aborting the previous request and + // discarding the rejection — there is no request counter — so an offline + // answer that resolves regardless of the signal can overwrite a newer list. + // The two cache reads are also very unequal: a prefix hit comes off the + // index, the substring pass walks every cached place of every trip. + if (signal?.aborted) throw abortedError() + return found } if (isEffectivelyOffline()) return shape(await fromCache()) @@ -1135,6 +1157,7 @@ export const mapsApi = { source: 'offline-cache', }), () => apiClient.post('/maps/autocomplete', { input, lang, locationBias, sessionToken }, { signal }).then(r => checkInDev(mapsAutocompleteResultSchema, r.data, 'maps.autocomplete')), + signal, ), // Answered from the cache when the network is not there, for the ids the // offline suggestion list hands out. Without it, picking an offline diff --git a/client/src/api/placeShadow.test.ts b/client/src/api/placeShadow.test.ts index ab2b74f3e..9ee99cd94 100644 --- a/client/src/api/placeShadow.test.ts +++ b/client/src/api/placeShadow.test.ts @@ -85,6 +85,23 @@ describe('recordPlacePick', () => { expect(cap.seen()).toBeNull() }) + it('does not record a pick made out of the offline cache', async () => { + // Such a row describes the local cache's ordering, not a provider's, and + // the two figures this corpus exists for — how often the live answer was + // first, and how often it was in the top five — are averaged over every + // row. Mixing these in moves exactly the number a candidate index is later + // judged against. Not merely an offline concern either: offline is a + // setting, so the network is usually reachable and the POST would land. + let posted = false + server.use(http.post('/api/place-shadow/pick', () => { posted = true; return HttpResponse.json({ recorded: true }) })) + + recordPlacePick({ ...PICK, source: 'search:offline-cache' }) + recordPlacePick({ ...PICK, source: 'autocomplete:offline-cache' }) + await new Promise(r => setTimeout(r, 20)) + + expect(posted).toBe(false) + }) + it('swallows a server error instead of surfacing it to the pick that triggered it', async () => { const cap = capture(500) const onUnhandled = vi.fn() diff --git a/client/src/api/placeShadow.ts b/client/src/api/placeShadow.ts index 73e1c7774..fae21700e 100644 --- a/client/src/api/placeShadow.ts +++ b/client/src/api/placeShadow.ts @@ -23,6 +23,15 @@ export function recordPlacePick(pick: PlaceShadowPickRequest): void { if (!useAuthStore.getState().placeShadowEnabled) return if (!Number.isFinite(pick.pickedLat) || !Number.isFinite(pick.pickedLng)) return if (pick.liveRank >= pick.liveCount) return + // A pick out of the offline cache describes the cache's ordering, not a + // provider's, and the two numbers this corpus exists for — how often the live + // answer was first, and how often it was in the top five — are averaged over + // every row. Mixing local rows in would move exactly the figure a candidate + // index is later judged against. + // + // Not merely an offline concern: the offline switch is a setting, so the + // network is usually reachable and the POST would go through. + if (pick.source.endsWith('offline-cache')) return const body: PlaceShadowPickRequest = { ...pick, diff --git a/client/src/sync/placePrefetcher.test.ts b/client/src/sync/placePrefetcher.test.ts index 3a61846d2..d69f9b84b 100644 --- a/client/src/sync/placePrefetcher.test.ts +++ b/client/src/sync/placePrefetcher.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import 'fake-indexeddb/auto' import { offlineDb, clearAll } from '../db/offlineDb' +import { setAuthed } from './authGate' import { AREA_PLACE_LIMIT, cachedToPlaceRecord, @@ -42,6 +43,9 @@ function apiRow(name: string, over: Record = {}) { beforeEach(async () => { await clearAll() + // The prefetch re-checks this after its request comes back, so a suite that + // never signs in would exercise the abandon path and nothing else. + setAuthed(true) areaMock.mockReset() areaMock.mockResolvedValue({ results: [apiRow("L'Osteria"), apiRow('Café Central')], truncated: false }) await offlineDb.syncMeta.put({ @@ -180,6 +184,20 @@ describe('searchCachedPlaces', () => { expect(found?.name).toBe("L'Osteria") }) + it('FE-PLACEPRE-018: a logout while the request is in flight leaves the database alone', async () => { + // The handle repoints at the anonymous database on logout, so writing the + // answer after that seeds one person's trip area into the next person's + // offline search. A shared tablet is all it takes. + await clearAll() + areaMock.mockImplementation(async () => { + setAuthed(false) + return { results: [apiRow('Nach dem Logout')], truncated: false } + }) + + await expect(prefetchPlacesForTrip(7, TRIP_PLACES, true)).resolves.toBe(0) + expect(await offlineDb.areaPlaces.count()).toBe(0) + }) + it('FE-PLACEPRE-017: an id the cache does not hold is a miss, not somebody else record', async () => { // A miss has to stay a miss: answering the wrong place would be worse than // the failure it replaces. diff --git a/client/src/sync/placePrefetcher.ts b/client/src/sync/placePrefetcher.ts index 86dabdc06..6ac66ef28 100644 --- a/client/src/sync/placePrefetcher.ts +++ b/client/src/sync/placePrefetcher.ts @@ -11,6 +11,7 @@ * trip's area actually changes. A trip to Rostock costs roughly a megabyte. */ import { mapsApi } from '../api/client' +import { isAuthed } from './authGate' import { offlineDb, type CachedAreaPlace } from '../db/offlineDb' import type { Place } from '../types' import { computeBbox } from './tilePrefetcher' @@ -120,6 +121,13 @@ export async function prefetchPlacesForTrip( const area = await mapsApi.area(bbox, AREA_PLACE_LIMIT) if (area.unavailable) return 0 + // Checked after the await, not only before it: the area request has a long + // timeout, and a logout in the meantime deletes the user's database and + // repoints the handle at the anonymous one. Writing then would seed one + // person's trip area into the next person's offline search — the same + // reason cacheFilesForTrip and the tile prefetch re-check here. + if (!isAuthed()) return 0 + const now = Date.now() const rows = (area.results || []) .map((r) => toCached(tripId, r as AreaRecord, now)) diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 824af20d8..c4515cad4 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -261,7 +261,13 @@ export interface OverpassPoi { phone: string | null; opening_hours: string | null; cuisine: string | null; - source: 'openstreetmap'; + /** + * Which index the row came from. Overture is not OpenStreetMap: it carries + * OSM among other sources under other licences, so a row from the TREK index + * says so rather than borrowing OSM's name. The wire contract keeps this an + * open string, so widening it here breaks nothing. + */ + source: 'openstreetmap' | 'trek-places'; } // Each pill category → the OSM tag selectors it searches. Keys here are the diff --git a/server/src/nest/maps/maps.mcp.ts b/server/src/nest/maps/maps.mcp.ts index 64602a422..6b1b126d1 100644 --- a/server/src/nest/maps/maps.mcp.ts +++ b/server/src/nest/maps/maps.mcp.ts @@ -89,7 +89,7 @@ export class MapsMcp { */ @Tool({ name: 'search_pois', - description: 'List OpenStreetMap points of interest of one category inside a map rectangle, with address, opening hours, website, phone and cuisine wherever OSM carries them. This is the discovery tool: use it to answer "what is around here" for a neighbourhood or a whole city district. Prefer search_place when the user already named the place they mean. Never calls Google, so it costs nothing and works on an instance with no Places key.', + description: 'List points of interest of one category inside a map rectangle, with address, opening hours, website, phone and cuisine wherever the source carries them. Answers from the TREK place index where it can and from OpenStreetMap otherwise; each result names which one it came from in its `source` field. This is the discovery tool: use it to answer "what is around here" for a neighbourhood or a whole city district. Prefer search_place when the user already named the place they mean. Never calls Google, so it costs nothing and works on an instance with no Places key.', inputSchema: { category: z.enum(POI_CATEGORY_KEYS).describe('Which kind of place to look for'), bbox: z.object({ diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 5ca68f2bf..ccfcda4cb 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -434,7 +434,13 @@ interface OverpassPoiElement { interface PoiSearchResult { pois: OverpassPoi[]; - source: 'openstreetmap'; + /** + * Which index answered. Not decoration: this branch calls naming the sources + * a licence obligation rather than a courtesy, and Overture carries more than + * OpenStreetMap under more than one licence, so stamping its rows as OSM is a + * wrong attribution rather than a rounding of one. + */ + source: 'openstreetmap' | 'trek-places'; truncated: boolean; // True when the requested viewport was too large and got shrunk to a centred // window before querying — the results then cover the middle of the view only. @@ -666,10 +672,17 @@ export class MapsService { }); if (found.length > 0) { // Same shape the Overpass path produces, so the client and the map - // renderer need no branch. `source` stays 'openstreetmap' because it - // is a typed contract the client switches on, and the places are the - // same places; where they came from is an implementation detail of - // this method, not something the pill should render differently. + // renderer need no branch — but named as what it is. Overture is not + // OpenStreetMap: it carries OSM among other sources under other + // licences, and this branch argues elsewhere that naming a source is + // a licence obligation. The wire contract keeps `source` an open + // string, so widening it costs nothing. + // + // One thing the index cannot do is localise. The Overpass path picks + // `name:` and falls back to `int_name`; the service has no + // language parameter at all, so a German user exploring Tokyo gets + // the Japanese primary names here. Named rather than hidden: whoever + // adds localisation upstream should find this comment. return { pois: found.map(p => ({ osm_id: `gers:${p.gers}`, @@ -685,9 +698,9 @@ export class MapsService { // The index has no cuisine field, so this null is the truth // rather than a field being dropped on the way through. cuisine: null, - source: 'openstreetmap' as const, + source: 'trek-places' as const, })), - source: 'openstreetmap' as const, + source: 'trek-places' as const, truncated: found.length >= 50, // A wide viewport is narrowed here too, and the caller is told so // for the same reason the Overpass path tells it. diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index e032b91dd..83e173141 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -11,7 +11,7 @@ * to configure and no quota to exhaust, so the failure modes are the network * and the service being down, both of which fall back to what TREK did before. */ -import { readEnv } from '../../app-config'; +import { readEnv, getAppUrl } from '../../app-config'; /** The public instance. An operator may point at their own copy instead. */ export const DEFAULT_TREK_PLACES_URL = 'https://places.liketrek.com'; @@ -89,7 +89,14 @@ export interface TrekPlacesSearchResponse { * identifies a person. It is never resolved back to anything on the far side. */ function instanceToken(): string { - const url = readEnv().app.appUrl || 'unconfigured'; + // getAppUrl(), not the raw APP_URL. The variable is commented out in the + // shipped compose file, so reading it raw gave every default install the same + // string — and a token every caller shares is not a caller. One install + // running a large import would then rate-limit all the others, who would fall + // back to Nominatim with nothing to point at. The resolver falls through to + // ALLOWED_ORIGINS and then to the port, so an unconfigured instance still + // differs from its neighbours. + const url = getAppUrl(); let hash = 0; for (let i = 0; i < url.length; i++) { hash = ((hash << 5) - hash + url.charCodeAt(i)) | 0; @@ -358,6 +365,11 @@ export function toPlaceRecord(p: TrekPlace): Record { // with the chain's logo and described as the company, which is the failure // readBrandIdentity and WIKI_IDENTITY_TAGS exist to prevent. 'brand:wikidata': p.brand?.wikidata ?? null, + // Carried rather than dropped: the enrichment wants exactly this field a + // moment later, and re-fetching the same place to get it made adding one + // place cost two full round trips to the index. Left as the service shaped + // it — the reader validates the URL before it becomes a link. + description: p.description ?? null, source: 'trek-places', }; } diff --git a/server/src/nest/place-enrichment/place-enrichment.service.ts b/server/src/nest/place-enrichment/place-enrichment.service.ts index 21cc9f9fc..050803397 100644 --- a/server/src/nest/place-enrichment/place-enrichment.service.ts +++ b/server/src/nest/place-enrichment/place-enrichment.service.ts @@ -586,17 +586,32 @@ export class PlaceEnrichmentService { * it from there. Only places carrying a GERS id can be looked up, which is * exactly the ones that came from the API in the first place. */ - private async websiteDescription(placeId: string): Promise { + private async websiteDescription( + placeId: string, + details?: Record | null, + ): Promise { if (!placeId.startsWith('gers:')) return null; try { - const place = await trekPlacesById(placeId.slice(5)); - const got = (place as { description?: { text?: string; sourceUrl?: string } } | null)?.description; + // The details lookup already fetched this place and now carries its + // description, so the ordinary path costs nothing. Asking again meant two + // full round trips to the index for one added place, each with its own + // timeout budget, for a field the first answer already contained. + const carried = (details as { description?: { text?: string; sourceUrl?: string } } | null | undefined) + ?.description; + const got = carried + ?? (await trekPlacesById(placeId.slice(5)) as { description?: { text?: string; sourceUrl?: string } } | null) + ?.description; const text = typeof got?.text === 'string' ? got.text.trim() : ''; if (!text) return null; + // Through the same allow-list a place's website goes through: this + // becomes an href on the client, and the value comes from whatever the + // configured index answered with. Anything but http(s) loses the link and + // keeps the text, rather than being rendered as one. + const rawUrl = typeof got?.sourceUrl === 'string' ? got.sourceUrl : null; return { text, source: 'website', - sourceUrl: typeof got?.sourceUrl === 'string' ? got.sourceUrl : null, + sourceUrl: rawUrl && placeWebsiteSchema.safeParse(rawUrl).success ? rawUrl : null, // Not a licensed corpus: a quoted summary from the operator's own page, // credited and linked back. Saying "CC-something" here would be a claim // about terms nobody granted. @@ -647,7 +662,7 @@ export class PlaceEnrichmentService { // exists, and it is published in JSON-LD or og:description precisely so // machines can read it. That covers roughly 43 percent of places, where the // encyclopaedias cover a fraction of a percent. - const fromSite = await this.websiteDescription(placeId); + const fromSite = await this.websiteDescription(placeId, details); if (fromSite) return fromSite; const apiKey = this.maps.getMapsKey(userId); diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index 280cf0eba..b9471660e 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -483,7 +483,11 @@ describe('Tool: search_pois', () => { // The index carries opening hours; dropping them would leave the tool // description promising something the answer never has. expect(data.pois[0].opening_hours).toBe('Mo-Fr 09:00-17:00'); - expect(data.source).toBe('openstreetmap'); + // The answer names the index that produced it, over MCP as over REST. An + // assistant reading `source` to attribute the data would otherwise credit + // OpenStreetMap for rows Overture carries under other licences. + expect(data.source).toBe('trek-places'); + expect(data.pois[0].source).toBe('trek-places'); expect(data.clamped).toBe(false); expect(MapsService.prototype.searchOverpassPois).not.toHaveBeenCalled(); diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index 7456b5f5d..02f0fdece 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -142,12 +142,15 @@ describe('MapsService.pois answered from the index', () => { phone: '+43 1 5333763', opening_hours: 'Mo-Sa 07:30-22:00', cuisine: null, - source: 'openstreetmap', + // Named as what it is. Overture is not OpenStreetMap — it carries OSM + // among other sources under other licences — and this branch argues + // elsewhere that naming a source is a licence obligation rather than a + // courtesy. Nothing on the client switches on the field, and the wire + // contract keeps it an open string, so saying so costs nothing. + source: 'trek-places', }, ]); - // 'trek-places' would be a third value for a field the client treats as a - // closed set, and these are the same places either way. - expect(out.source).toBe('openstreetmap'); + expect(out.source).toBe('trek-places'); }); it('MAPS-POIS-002: a row without address, contact or hours becomes nulls, typed as the pill', async () => { @@ -169,7 +172,7 @@ describe('MapsService.pois answered from the index', () => { phone: null, opening_hours: null, cuisine: null, - source: 'openstreetmap', + source: 'trek-places', }); }); @@ -203,7 +206,7 @@ describe('MapsService.pois answered from the index', () => { phone: null, opening_hours: 'Mo-Sa 07:30-22:00', cuisine: null, - source: 'openstreetmap', + source: 'trek-places', }); }); diff --git a/server/tests/unit/nest/place-enrichment.description.test.ts b/server/tests/unit/nest/place-enrichment.description.test.ts index 6f373b619..e0db2a534 100644 --- a/server/tests/unit/nest/place-enrichment.description.test.ts +++ b/server/tests/unit/nest/place-enrichment.description.test.ts @@ -362,6 +362,56 @@ describe("the description on the place's own website", () => { lang: 'de', }; + it('ENRICH-100: reads the description the details lookup already fetched', async () => { + // The details call fetched this place a moment ago and its answer carries + // the description. Asking again made adding one place cost two full round + // trips to the index, each with its own timeout, for a field already in + // hand. + mockTrekPlacesById.mockClear(); + + const out = await make(mapsStub()).enrich(1, { + ...GERS_REQ, + details: { description: { text: 'Pizza in Rostock, seit 2015.', sourceUrl: 'https://losteria.net/rostock' } }, + }); + + expect(out.description).toMatchObject({ text: 'Pizza in Rostock, seit 2015.', source: 'website' }); + expect(mockTrekPlacesById).not.toHaveBeenCalled(); + }); + + it('ENRICH-101: still asks when the caller passed no details', async () => { + mockTrekPlacesById.mockClear(); + mockTrekPlacesById.mockResolvedValue({ + description: { text: 'Pizza in Rostock, seit 2015.', sourceUrl: 'https://losteria.net/rostock' }, + }); + + const out = await make(mapsStub()).enrich(1, GERS_REQ); + + expect(out.description).toMatchObject({ source: 'website' }); + expect(mockTrekPlacesById).toHaveBeenCalledWith('abc-123'); + }); + + it('ENRICH-102: a source URL that is not http(s) loses the link, not the text', async () => { + // It becomes an href on the client, and the value comes from whatever index + // the instance is pointed at. The same allow-list a place's website goes + // through, for the same reason. + mockTrekPlacesById.mockClear(); + + for (const bad of ['javascript:alert(1)', 'data:text/html,