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,