diff --git a/CHANGELOG.md b/CHANGELOG.md index 40c545a2a..231ab050e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,12 @@ of current runtime behavior, see [`docs/CURRENT-STATE.md`](docs/CURRENT-STATE.md - Existing cached refusals are now ignored immediately, including during stale-data fallback. Concurrent identical requests share the same last-good fallback when all mirrors refuse, without duplicating upstream requests. +- A keyless place lookup no longer remembers a network failure as "no such + place". A blip while Photon was answering used to be memoized for the rest of + the session, so the query kept returning not-found from memory on a network + that had since recovered. A miss is now cached only when every source + consulted actually returned a verdict. + ### Added - Keyless place search. The LOCATION search box and the `fly_to_location` voice @@ -25,6 +31,13 @@ of current runtime behavior, see [`docs/CURRENT-STATE.md`](docs/CURRENT-STATE.md the primary path and is unchanged when it answers; the fallback also covers a key whose Geocoding API is not enabled, which Google reports as HTTP 200 with `REQUEST_DENIED`, so an empty result is the detector rather than an error. +- The same keyless fallback now covers the remaining two place lookups: map + annotations ("annotate the botanical garden") and the Radio layer's + "near \" selection. Radio previously threw without a key, which + surfaced as a failed voice turn rather than as a station it could not place; + annotations silently failed to anchor. Annotation footprints match OSM on the + resolved feature's canonical name, so locality words in the request cannot + pull the outline onto a neighbouring building. ## [0.1.1] — 2026-09-01 — Installation and live-data fixes diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 00003a260..6fcd02b0c 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -2183,7 +2183,11 @@ silently demoting every later lookup for the session. ### Keyless place search (August 2026) -- `src/keylessGeocoder.js` is a Photon (komoot/OpenStreetMap) adapter used by `searchAndFlyTo` when the Google geocode produces no result: no key configured, or a key whose Geocoding API is not enabled — Google answers HTTP 200 with `REQUEST_DENIED`, so the empty result is the detector, not an error. Google stays the primary path and is byte-unchanged when it answers, and the Places near-view recovery is only attempted when a key exists, since Places is a Google service. +- `src/keylessGeocoder.js` is a Photon (komoot/OpenStreetMap) adapter used when the Google geocode produces no result: no key configured, or a key whose Geocoding API is not enabled — Google answers HTTP 200 with `REQUEST_DENIED`, so the empty result is the detector, not an error. Google stays the primary path and is byte-unchanged when it answers, and the Places near-view recovery is only attempted when a key exists, since Places is a Google service. +- The fallback covers **all three** client call sites that forward-geocode a name, not just the search box: `searchAndFlyTo` (`src/locations.js`), the annotation resolver's `geocodePlace` (`src/annotations/annotationResolver.js`) and the Radio layer's "near \" lookup (`resolveRadioLocation`, `src/voice/gevActions.js`). The last of those used to THROW without a key, which surfaced as a failed voice turn rather than as a station it could not place. `src/keylessGeocoder.test.mjs` pins the inventory by counting the call sites, so a second seam added to a file that already has a fallback fails the suite too. Reverse geocoding (`?latlng=`) is deliberately NOT covered: the street-label reader consumes Google's ranked `address_components`, and Photon's `/reverse` cannot reproduce that shape. +- For the annotation resolver the adapter also carries the feature's own `name`. OSM footprint scoring matches on that canonical name rather than on the user's words, so incidental locality tokens cannot win — "the monument near the Texas Capitol, Austin" scored on the raw utterance picks a downtown building that merely shares "Texas", "Capitol" and "Austin" (the same failure as field test 7). +- Photon's `country` is NOT re-exported as a field. It comes back in the feature's own language ("Việt Nam", "日本", "Россия"), and `normalizeRadioCountryInput` fails CLOSED on a name it cannot map — forwarding it would filter the station list to nothing for exactly the places the lookup just resolved. The composed label still names the country. +- A not-found is remembered only when every source consulted returned a verdict (`geocodeMissIsDefinitive`). Photon not replying is a network blip, not "no such place": caching it would keep an annotation unplaceable for the rest of the session on a network that has since recovered — the same conflation the Overpass disk cache had to be repaired for. The adapter's own memo follows the same rule, so an unanswered lookup is retried rather than replayed. - The adapter normalizes into the shape the Google path already produces — `lat`/`lng`, label, Google-style `types`, `{southwest,northeast}` bounds — so `geocodeNavigationMode`, `regionFramingPlan` and the region-swath cap keep running unchanged. Two translations are load-bearing and are pinned by tests: Photon's `extent` is `[west, north, east, south]`, and a specific OSM `key=value` rule beats the coarse `type` (Photon reports a lake as `water=lake` with `type: 'other'`, which framed at building range put the camera 26 m over the water; it reports a town square as `type: 'locality'`, which framed a plaza as a city). - The shared `viewportBias()` string is sent as Photon's `lat`/`lon` PROXIMITY bias, never as `bbox`: Google's `bounds` prefers, Photon's `bbox` filters, and the literal translation makes every off-screen search return nothing. One result per query, memoized by query+bias (misses included) to respect komoot's fair use. diff --git a/src/annotations/annotationResolver.js b/src/annotations/annotationResolver.js index 3bcc143c2..dce8fc2e0 100644 --- a/src/annotations/annotationResolver.js +++ b/src/annotations/annotationResolver.js @@ -2,6 +2,7 @@ import * as Cesium from 'cesium'; import { lookupNeighborhoodRing } from '../data/neighborhoodPolygons.js'; import { lookupNaturalRegionOutline, findNaturalRegion } from '../data/naturalEarthRegions.js'; import { registerDynamicCredit, NATURAL_EARTH_CREDIT } from '../data/dataCredits.js'; +import { geocodeKeylessWithOutcome } from '../keylessGeocoder.js'; import { isPickedWorldPosition } from '../data/scenePick.js'; /** @@ -599,17 +600,19 @@ function ringAreaM2(ring) { } /** - * Forward-geocode a place name via Google Geocoding, biased to the current - * viewport so "the marina" resolves near where the user is looking. + * Forward-geocode a place name, biased to the current viewport so "the marina" + * resolves near where the user is looking. Google first; Photon when there is + * no key, or when Google returns nothing. */ async function geocodePlace(query, biasRect, signal) { const apiKey = window.__GOOGLE_MAPS_API_KEY__ || import.meta.env.GOOGLE_MAPS_API_KEY; - if (!apiKey) return null; const cacheKey = `${query.toLowerCase()}|${biasRect || ''}`; const cached = cacheRead(geocodeCache, cacheKey); if (cached !== undefined) return cached; + if (!apiKey) return keylessPlace(query, biasRect, cacheKey, signal, { googleConsulted: false }); + let url = `https://maps.googleapis.com/maps/api/geocode/json?address=${encodeURIComponent(query)}&key=${apiKey}`; if (biasRect) url += `&bounds=${biasRect}`; @@ -617,10 +620,13 @@ async function geocodePlace(query, biasRect, signal) { const response = await fetch(url, { signal }); const data = await response.json(); if (data.status !== 'OK' || !data.results?.length) { - // ZERO_RESULTS is a definitive not-found (cacheable); OVER_QUERY_LIMIT / - // REQUEST_DENIED / UNKNOWN_ERROR are transient → don't poison the cache. - negCache(geocodeCache, cacheKey, signal, data?.status === 'ZERO_RESULTS'); - return null; + // Google found nothing, or declined. An unenabled Geocoding API answers + // HTTP 200 with REQUEST_DENIED, so this branch is also how a + // half-configured key looks — either way the keyless path is the answer. + return keylessPlace(query, biasRect, cacheKey, signal, { + googleConsulted: true, + googleSaysNoSuchPlace: data?.status === 'ZERO_RESULTS', + }); } const result = data.results[0]; const place = { @@ -644,6 +650,64 @@ async function geocodePlace(query, biasRect, signal) { } } +/** + * Whether a combined Google+Photon not-found may be REMEMBERED as "no such + * place", or must be left open for the next attempt. + * + * One rule for both entry points: a miss is definitive only when EVERY source + * that was consulted returned a verdict. With no key Photon is the only source, + * so its answer decides. With a key, Google declining (REQUEST_DENIED on an + * unenabled API, OVER_QUERY_LIMIT) or Photon being unreachable each leave the + * question open — and Photon holds plenty of places Google does not, which is + * the reason this fallback exists at all. Caching either would keep an + * annotation unplaceable for the rest of the session on a network that has + * since recovered. + * + * Exported for the unit tests: the no-key branch cannot be driven from + * `node --test`, because the key expression reads `import.meta.env`. + * @param {{photonAnswered:boolean, googleConsulted:boolean, + * googleSaysNoSuchPlace?:boolean}} sources + * @returns {boolean} + */ +export function geocodeMissIsDefinitive({ photonAnswered, googleConsulted, googleSaysNoSuchPlace = false }) { + if (!photonAnswered) return false; + return !googleConsulted || Boolean(googleSaysNoSuchPlace); +} + +/** + * Photon fallback for `geocodePlace`, in the shape the annotation pipeline + * already consumes. + * + * `primaryName` is the feature's own name rather than a name extracted from a + * formatted address, which is what the OSM name-matching downstream wants — + * Photon reports it directly, so nothing has to be parsed back out of a label. + * @param {string} query Free-text place name. + * @param {?string} biasRect `"swLat,swLng|neLat,neLng"` viewport bias. + * @param {string} cacheKey Shared cache key, so both paths memoise together. + * @param {?AbortSignal} signal Caller's abort signal. + * @param {{googleConsulted:boolean, googleSaysNoSuchPlace?:boolean}} google What + * the Google leg contributed: whether it ran at all, and whether it answered + * ZERO_RESULTS rather than declining. + * @returns {Promise} Resolved place, or null. + */ +async function keylessPlace(query, biasRect, cacheKey, signal, google) { + const { place: keyless, answered } = await geocodeKeylessWithOutcome(query, { bias: biasRect || null }); + if (!keyless) { + negCache(geocodeCache, cacheKey, signal, geocodeMissIsDefinitive({ ...google, photonAnswered: answered })); + return null; + } + const place = { + lat: keyless.lat, + lon: keyless.lng, + label: shortLabel(keyless.label), + primaryName: keyless.name || null, + types: keyless.types, + viewport: normalizeGeocodeViewport(keyless.viewport), + }; + cacheWrite(geocodeCache, cacheKey, place); + return place; +} + /** Geocoding returns {southwest:{lat,lng},northeast:{lat,lng}}; normalize to the Places * {low,high} lat/lng shape the rest of the pipeline (disc sizing, framing) consumes. */ function normalizeGeocodeViewport(vp) { diff --git a/src/annotations/annotationResolver.test.mjs b/src/annotations/annotationResolver.test.mjs index 1ab378683..0741b56b8 100644 --- a/src/annotations/annotationResolver.test.mjs +++ b/src/annotations/annotationResolver.test.mjs @@ -12,6 +12,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { + geocodeMissIsDefinitive, resolveAnnotationTarget, selectFootprint, refineScope, @@ -362,3 +363,186 @@ test('ask-side admin bypass: admin level 2/3 result types never grant a township 'both township-level admin result types stay guarded', ); }); + +// ── Keyless geocoding ──────────────────────────────────────────────────────── +// +// `geocodePlace` was Google-only: no key returned null, and so did a key whose +// Geocoding API is not enabled (Google answers HTTP 200 with REQUEST_DENIED). +// Either way the annotation silently failed to place. Both now fall through to +// Photon. These drive the second case, because it reaches the SAME fallback +// through a running Google branch — the no-key branch cannot be driven from +// `node --test`, since the key expression reads `import.meta.env`, which only +// Vite defines. + +/** Photon's GeoJSON shape, trimmed to the properties the adapter consumes. */ +function photonFeature({ name, lat, lon, tags = {}, extent = null, ...rest }) { + return { + geometry: { type: 'Point', coordinates: [lon, lat] }, + properties: { name, ...tags, ...rest, ...(extent ? { extent } : {}) }, + }; +} + +const GOOGLE_FOUND_NOTHING = { ok: true, json: async () => ({ status: 'ZERO_RESULTS', results: [] }) }; + +test('keyless: a key that geocodes to nothing still anchors the annotation', async (t) => { + const requests = []; + installGoogleMocks(t, async (url) => { + requests.push(String(url)); + if (String(url).startsWith('https://maps.googleapis.com/')) return GOOGLE_FOUND_NOTHING; + return { + ok: true, + json: async () => ({ + features: [photonFeature({ + name: 'Lady Bird Lake', + lat: 30.25, + lon: -97.73, + city: 'Austin', + state: 'Texas', + country: 'United States', + tags: { osm_key: 'natural', osm_value: 'water' }, + // Photon orders extent [west, north, east, south] — a naive [w,s,e,n] + // read would invert the box and still look like a valid viewport. + extent: [-97.8, 30.28, -97.68, 30.24], + })], + }), + }; + }); + + const resolved = await resolveAnnotationTarget({ + viewer: closeViewportViewer(), + target: 'Lady Bird Lake', + }); + + assert.ok(resolved, 'a Google miss must not leave the annotation unplaced'); + assert.equal(resolved.source, 'geocode'); + assert.deepEqual([resolved.lat, resolved.lon], [30.25, -97.73]); + // The label is shortened the same way a Google `formatted_address` is. + assert.equal(resolved.label, 'Lady Bird Lake'); + // Photon's extent reaches the framing code in the Places `{low,high}` shape, + // right way up — this is what sizes a grounds disc and the flyTo box. + assert.deepEqual(resolved.viewport, { + low: { latitude: 30.24, longitude: -97.8 }, + high: { latitude: 30.28, longitude: -97.68 }, + }); + // Google is asked first and exactly once; one Photon call answers it. + assert.equal(requests.filter((url) => url.includes('maps.googleapis.com')).length, 1); + assert.equal(requests.filter((url) => url.includes('photon.komoot.io')).length, 1); +}); + +test('keyless: OSM matching uses the feature\'s canonical name, not the user\'s words', async (t) => { + // The reason `primaryName` is carried out of Photon at all. The ask names a + // landmark AND its surroundings; the geocoded feature is called just "Tejano + // Monument". Score the OSM candidates on the raw utterance and the locality + // tokens win — this is the Thompson-Austin bug (field test 7) reached through + // the keyless path. Score them on the canonical name and the monument wins. + const decoy = squareWay(ANCHOR, -250, -250, 3000, { + building: 'yes', tourism: 'hotel', name: 'Texas Capitol Austin Visitor Center', + }); + const monument = squareWay(ANCHOR, 5, 5, 300, { tourism: 'artwork', name: 'Tejano Monument' }); + + installGoogleMocks(t, async (url, init) => { + const href = String(url); + if (href.startsWith('https://maps.googleapis.com/')) return GOOGLE_FOUND_NOTHING; + if (href.startsWith('/api/google/text-search')) return { ok: true, json: async () => ({ places: [] }) }; + if (href.startsWith('https://photon.komoot.io/')) { + return { + ok: true, + json: async () => ({ + features: [photonFeature({ + name: 'Tejano Monument', + lat: ANCHOR.lat, + lon: ANCHOR.lon, + city: 'Austin', + tags: { osm_key: 'historic', osm_value: 'memorial' }, + })], + }), + }; + } + assert.equal(href, '/api/overpass'); + assert.equal(init?.method, 'POST'); + return { ok: true, status: 200, headers: { get: () => null }, json: async () => ({ elements: [decoy, monument] }) }; + }); + + const resolved = await resolveAnnotationTarget({ + viewer: closeViewportViewer(), + target: 'the monument near the Texas Capitol, Austin', + footprint: true, + }); + + assert.ok(resolved.ring, 'the monument polygon is monument-scale and exactly named — it must outline'); + // The anchor re-centres on the chosen polygon, so its position IS the verdict: + // the monument sits on the anchor, the decoy 354 m south-west of it. + assert.ok( + approximateMetres(resolved.lat, resolved.lon, ANCHOR.lat, ANCHOR.lon) < 50, + `outlined a polygon ${Math.round(approximateMetres(resolved.lat, resolved.lon, ANCHOR.lat, ANCHOR.lon))} m from the monument`, + ); +}); + +/** Metres between two coordinates — enough precision for a few hundred metres. */ +function approximateMetres(lat1, lon1, lat2, lon2) { + const mLat = 111320; + const dy = (lat1 - lat2) * mLat; + const dx = (lon1 - lon2) * mLat * Math.cos((lat1 * Math.PI) / 180); + return Math.hypot(dx, dy); +} + +test('keyless: a Photon outage is not remembered as "no such place"', async (t) => { + // Google answering ZERO_RESULTS is a verdict about GOOGLE. Photon holds + // plenty of places Google does not — that asymmetry is why this fallback + // exists — so a Google verdict plus a Photon timeout must leave the query + // open. Caching it would keep an annotation unplaceable for the rest of the + // session, on a network that has since recovered. + const photonUp = { + ok: true, + json: async () => ({ + features: [photonFeature({ + name: 'Waller Creek', lat: 30.2665, lon: -97.7385, city: 'Austin', + tags: { osm_key: 'waterway', osm_value: 'stream' }, + })], + }), + }; + let photonReachable = false; + const requests = []; + installGoogleMocks(t, async (url) => { + requests.push(String(url)); + if (String(url).startsWith('https://maps.googleapis.com/')) return GOOGLE_FOUND_NOTHING; + if (!photonReachable) throw new Error('network down'); + return photonUp; + }); + + const target = 'Waller Creek'; + assert.equal(await resolveAnnotationTarget({ viewer: closeViewportViewer(), target }), null); + const duringOutage = requests.length; + + photonReachable = true; + const retried = await resolveAnnotationTarget({ viewer: closeViewportViewer(), target }); + + assert.ok(retried, 'the same target must resolve once the network is back'); + assert.deepEqual([retried.lat, retried.lon], [30.2665, -97.7385]); + assert.ok( + requests.length > duringOutage, + 'the retry was served from a poisoned cache instead of asking again', + ); +}); + +test('a not-found is remembered only when every source consulted gave a verdict', () => { + // The full truth table, because the no-key row is the one the behavioural + // tests above cannot reach: `import.meta.env` only exists under Vite, so + // `node --test` can never take the branch that skips Google entirely. + const cases = [ + // no key: Photon is the only source, so Photon alone decides + [{ googleConsulted: false, photonAnswered: true }, true], + [{ googleConsulted: false, photonAnswered: false }, false], + // with a key: Google must have said "no such place", AND Photon must have replied + [{ googleConsulted: true, googleSaysNoSuchPlace: true, photonAnswered: true }, true], + [{ googleConsulted: true, googleSaysNoSuchPlace: true, photonAnswered: false }, false], + // Google declined (REQUEST_DENIED / OVER_QUERY_LIMIT) — never a verdict + [{ googleConsulted: true, googleSaysNoSuchPlace: false, photonAnswered: true }, false], + [{ googleConsulted: true, googleSaysNoSuchPlace: false, photonAnswered: false }, false], + ]; + for (const [sources, expected] of cases) { + assert.equal(geocodeMissIsDefinitive(sources), expected, JSON.stringify(sources)); + } + // An omitted Google verdict defaults to "did not say no such place". + assert.equal(geocodeMissIsDefinitive({ googleConsulted: true, photonAnswered: true }), false); +}); diff --git a/src/keylessGeocoder.js b/src/keylessGeocoder.js index 8b8b24e4a..8f2f89413 100644 --- a/src/keylessGeocoder.js +++ b/src/keylessGeocoder.js @@ -157,10 +157,21 @@ export function photonResultLabel(properties) { } /** - * Photon GeoJSON feature → the normalized geocode result `searchAndFlyTo` - * consumes. Returns null for a feature without usable coordinates. + * Photon GeoJSON feature → the normalized geocode result its callers consume. + * Returns null for a feature without usable coordinates. + * + * `name` is carried alongside the composed `label` because two call sites want + * different halves of the same answer: the search box shows the full label, + * while the annotation resolver matches OSM features on the canonical name + * alone. Deriving it here keeps one shape rather than two parsers of the same + * string. + * + * `properties.country` is deliberately NOT re-exported. Photon answers in the + * feature's own language ("Việt Nam", "日本", "Россия"), which no consumer in + * this app can match; the composed label is the only honest use for it. * @param {object} feature - One entry of Photon's `features` array. - * @returns {?{lat:number, lng:number, label:string, types:string[], viewport:?object}} + * @returns {?{lat:number, lng:number, name:string, label:string, + * types:string[], viewport:?object}} */ export function normalizePhotonFeature(feature) { const [lng, lat] = feature?.geometry?.coordinates || []; @@ -168,10 +179,12 @@ export function normalizePhotonFeature(feature) { if (Math.abs(lat) > 90 || Math.abs(lng) > 180) return null; const properties = feature.properties || {}; + const name = String(properties.name || ''); return { lat, lng, - label: photonResultLabel(properties) || String(properties.name || ''), + name, + label: photonResultLabel(properties) || name, types: photonResultTypes(properties), viewport: photonExtentToBounds(properties.extent), }; @@ -266,20 +279,33 @@ function trimPhotonCache() { } /** - * Geocode a place name without any API key. Never throws: a network failure, - * a timeout, or an empty result all resolve to null, which the caller reads as - * "not found" exactly as it reads a Google miss. + * Geocode a place name without any API key, reporting whether the service + * ANSWERED as well as what it found. + * + * A caller that remembers negative results needs both halves. "Photon has no + * such place" is a verdict and may be cached; "Photon did not reply" is a + * network blip, and caching THAT keeps the app answering "not found" from + * memory long after the network came back — a whole session, for a key that + * would now resolve. Conflating the two is the defect this app already had to + * repair once in the Overpass disk cache, where refusals were persisted with + * normal data TTLs. + * + * Never throws: a failure, a timeout, and an empty result all resolve to a null + * `place`, which the caller reads exactly as it reads a Google miss. * @param {string} query - Free-text place name. * @param {{bias?: ?string, fetchImpl?: Function}} [options] - * @returns {Promise} + * @returns {Promise<{place: ?{lat:number, lng:number, name:string, label:string, + * types:string[], viewport:?object}, answered:boolean}>} */ -export async function geocodeKeyless(query, { bias = null, fetchImpl = fetch } = {}) { +export async function geocodeKeylessWithOutcome(query, { bias = null, fetchImpl = fetch } = {}) { const trimmed = String(query ?? '').trim(); - if (!trimmed) return null; + if (!trimmed) return { place: null, answered: true }; const memoKey = `${bias ?? ''}\n${trimmed}`; - if (photonCache.has(memoKey)) return photonCache.get(memoKey); + // Only answered outcomes are memoised, so a hit is always an answer. + if (photonCache.has(memoKey)) return { place: photonCache.get(memoKey), answered: true }; + /** One request: the feature array Photon returned, or null if it did not answer. */ const ask = async (url) => { try { const response = await fetchImpl(url, { signal: AbortSignal.timeout(PHOTON_TIMEOUT_MS) }); @@ -298,24 +324,49 @@ export async function geocodeKeyless(query, { bias = null, fetchImpl = fetch } = // with the name asked for; otherwise the same query runs unbiased, which is // what a user searching for somewhere off-screen meant in the first place. let feature = null; + let everyAskAnswered = true; if (bias) { - feature = selectPhotonFeature(await ask(photonSearchUrl(trimmed, { bias })), normalizeToponym(trimmed)); + const biased = await ask(photonSearchUrl(trimmed, { bias })); + if (biased === null) everyAskAnswered = false; + else feature = selectPhotonFeature(biased, normalizeToponym(trimmed)); } if (!feature) { const anywhere = await ask(photonSearchUrl(trimmed, { bias: null })); - // "Hoan Kiem Lake, Hanoi" names a place and then the region holding it, the - // convention every geocoder's free-text field follows. The head segment is - // the thing being searched for; the tail only says where to look. - const head = normalizeToponym(trimmed.split(',')[0]); - feature = selectPhotonFeature(anywhere, head, { allowContains: true }) || anywhere?.[0]; + if (anywhere === null) everyAskAnswered = false; + else { + // "Hoan Kiem Lake, Hanoi" names a place and then the region holding it, the + // convention every geocoder's free-text field follows. The head segment is + // the thing being searched for; the tail only says where to look. + const head = normalizeToponym(trimmed.split(',')[0]); + feature = selectPhotonFeature(anywhere, head, { allowContains: true }) || anywhere[0]; + } } - const result = normalizePhotonFeature(feature); + const place = normalizePhotonFeature(feature); + // A found place is self-evidently an answer; a null one only counts as an + // answer when every request it rests on actually came back. + const answered = Boolean(place) || everyAskAnswered; - // Misses are cached too: a typo re-issued on every keystroke should cost one - // request, not one per stroke. - photonCache.set(memoKey, result); - trimPhotonCache(); - return result; + // Answered misses are cached too: a typo re-issued on every keystroke should + // cost one request, not one per stroke. Unanswered ones are not, so the next + // attempt retries instead of replaying a network blip. + if (answered) { + photonCache.set(memoKey, place); + trimPhotonCache(); + } + return { place, answered }; +} + +/** + * Geocode a place name without any API key. Never throws: a network failure, a + * timeout, or an empty result all resolve to null, which the caller reads as + * "not found" exactly as it reads a Google miss. + * @param {string} query - Free-text place name. + * @param {{bias?: ?string, fetchImpl?: Function}} [options] + * @returns {Promise} + */ +export async function geocodeKeyless(query, options) { + return (await geocodeKeylessWithOutcome(query, options)).place; } diff --git a/src/keylessGeocoder.test.mjs b/src/keylessGeocoder.test.mjs index b7e0a0078..74f0a06bb 100644 --- a/src/keylessGeocoder.test.mjs +++ b/src/keylessGeocoder.test.mjs @@ -17,8 +17,12 @@ // Run with: npm test import { test } from 'node:test'; import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { geocodeKeyless, + geocodeKeylessWithOutcome, normalizePhotonFeature, normalizeToponym, photonExtentToBounds, @@ -29,6 +33,8 @@ import { } from './keylessGeocoder.js'; import { geocodeNavigationMode } from './locations.js'; +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); + /** Photon's real answer for "Hanoi", trimmed to the fields we consume. */ const HANOI = { geometry: { type: 'Point', coordinates: [105.854041, 21.0283334] }, @@ -288,3 +294,182 @@ test('accents and punctuation do not change what a name compares as', () => { assert.equal(normalizeToponym(" St. John's-Ravenscourt "), 'st john s ravenscourt'); assert.equal(normalizeToponym(undefined), ''); }); + +// ── The seam inventory ─────────────────────────────────────────────────────── +// +// A fallback is only worth having where the app actually geocodes, and the app +// geocodes in more than one place: the LOCATION search box, the Radio layer's +// "near " lookup, and the annotation resolver. Fixing one and leaving +// the others is the failure this pins — the first version of this work fixed +// the search box alone, so a keyless install still threw on "play radio near +// Hanoi" while the search box beside it worked. +// +// It reads the source rather than the behaviour on purpose. The defect is not +// that a particular function is wrong; it is that a NEW call site can be added +// without one, and no behavioural test of the existing three would notice. + +/** Client modules that forward-geocode a place name through Google. */ +const GOOGLE_FORWARD_GEOCODE = /maps\.googleapis\.com\/maps\/api\/geocode\/json\?address/; +const CLIENT_SOURCES = fs.readdirSync(path.join(ROOT, 'src'), { recursive: true }) + .filter((entry) => typeof entry === 'string' && entry.endsWith('.js')) + .map((entry) => ({ file: path.join('src', entry), body: fs.readFileSync(path.join(ROOT, 'src', entry), 'utf8') })); + +/** + * Every forward-geocode call site in the client, by file. Reverse geocoding + * (`?latlng=`) is deliberately absent: Google returns ranked + * `address_components` that the street-label reader consumes, and Photon's + * `/reverse` cannot reproduce that shape — an honest gap, not an oversight. + */ +const FORWARD_GEOCODE_SEAMS = Object.freeze({ + 'src/locations.js': 1, + 'src/annotations/annotationResolver.js': 1, + 'src/voice/gevActions.js': 1, +}); + +test('the seam inventory is exactly the call sites we know about', () => { + // Counted, not merely listed. A file-level check would pass a SECOND seam + // added to a file that already has a fallback — which is how the first + // version of this work shipped: the search box had one, and the two call + // sites beside it did not. + assert.ok(CLIENT_SOURCES.length > 50, `only ${CLIENT_SOURCES.length} client sources scanned`); + const found = {}; + for (const { file, body } of CLIENT_SOURCES) { + const count = (body.match(new RegExp(GOOGLE_FORWARD_GEOCODE, 'g')) || []).length; + if (count) found[file] = count; + } + // A new seam is not a failure — it is a prompt to give it the same fallback + // and record it here. + assert.deepEqual(found, { ...FORWARD_GEOCODE_SEAMS }); +}); + +test('every place that forward-geocodes through Google can also answer without a key', () => { + for (const { file, body } of CLIENT_SOURCES) { + if (!GOOGLE_FORWARD_GEOCODE.test(body)) continue; + assert.match(body, /geocodeKeyless/, `${file} geocodes through Google with no keyless fallback`); + } +}); + +test('a missing key is never a thrown error on the client', () => { + // It was one, in the Radio layer: a keyless install surfaced "play radio near + // Hanoi" as a failed voice turn rather than as a station it could not place. + for (const { file, body } of CLIENT_SOURCES) { + assert.doesNotMatch(body, /throw new Error\([^)]*No Google Maps API key/, `${file} throws on a missing key`); + } +}); + +// ── The shape the three call sites share ───────────────────────────────────── + +test('the normalized result carries the canonical name its callers need', () => { + // Two consumers want different halves of one answer: the search box shows + // `label`, while the annotation resolver matches OSM features on `name` + // alone — a canonical name beats a parsed label there, because "Hà Nội, + // Việt Nam" would let the country token win the footprint scoring. + const result = normalizePhotonFeature(HANOI); + + assert.equal(result.name, 'Hà Nội'); + assert.equal(result.label.startsWith('Hà Nội'), true); + assert.equal(result.label.includes(result.name), true, 'the label is built from the name, not beside it'); +}); + +test('a feature with no name yields an empty string, never undefined', () => { + const bare = { geometry: { coordinates: [105.85, 21.03] }, properties: { osm_key: 'place', osm_value: 'city' } }; + const result = normalizePhotonFeature(bare); + + assert.equal(result.name, ''); + assert.equal(typeof result.label, 'string'); +}); + +test("Photon's own country name stays inside the label — it is not a field", () => { + // Photon answers in the feature's LANGUAGE: this fixture is the live service's + // real reply for "Hanoi", and it says "Việt Nam", not "Vietnam". Tokyo answers + // "日本", Moscow "Россия". Re-exporting that as a `country` field invites a + // consumer to match on it, and the one consumer that would — the Radio + // layer's `normalizeRadioCountryInput` — fails CLOSED on a name it cannot + // map, turning "radio near Ha Long" into an empty station list. The composed + // label is the only honest use for a localized country name. + assert.equal(HANOI.properties.country, 'Việt Nam'); + const result = normalizePhotonFeature(HANOI); + + assert.equal('country' in result, false, 'a localized country name must not become a matchable field'); + assert.match(result.label, /Việt Nam$/); +}); + +// ── An answer and a silence are not the same miss ──────────────────────────── +// +// Both arrive as `null`, and remembering the wrong one is the defect this app +// has already had to repair once: the Overpass disk cache persisted refusals +// with normal data TTLs, so an install that hit a bad minute kept serving the +// refusal for a month. A geocoder that memoises a network blip does the same +// thing for a session — every later keystroke replays it without asking. + +test('an unanswered request is never memoized, so the next attempt really retries', async () => { + let attempts = 0; + const offline = () => { attempts += 1; return Promise.reject(new Error('offline')); }; + const online = () => { + attempts += 1; + return Promise.resolve({ ok: true, json: () => Promise.resolve({ features: [HANOI] }) }); + }; + + assert.equal(await geocodeKeyless('blip probe', { fetchImpl: offline }), null); + assert.equal(attempts, 1); + + const recovered = await geocodeKeyless('blip probe', { fetchImpl: online }); + assert.equal(attempts, 2, 'the retry must reach the network, not a memoized blip'); + assert.equal(recovered.name, 'Hà Nội'); +}); + +test('an HTTP refusal counts as silence, not as "no such place"', async () => { + let attempts = 0; + const refusing = () => { + attempts += 1; + return Promise.resolve({ ok: false, status: 429, json: () => Promise.resolve({}) }); + }; + + await geocodeKeyless('throttle probe', { fetchImpl: refusing }); + await geocodeKeyless('throttle probe', { fetchImpl: refusing }); + assert.equal(attempts, 2, 'a 429 must not be remembered as a verdict'); +}); + +test('the outcome reports which of the two a null result was', async () => { + const answeredMiss = () => Promise.resolve({ ok: true, json: () => Promise.resolve({ features: [] }) }); + const failure = () => Promise.reject(new Error('offline')); + const hit = () => Promise.resolve({ ok: true, json: () => Promise.resolve({ features: [HANOI] }) }); + + assert.deepEqual( + await geocodeKeylessWithOutcome('outcome probe a', { fetchImpl: answeredMiss }), + { place: null, answered: true }, + ); + assert.deepEqual( + await geocodeKeylessWithOutcome('outcome probe b', { fetchImpl: failure }), + { place: null, answered: false }, + ); + + const found = await geocodeKeylessWithOutcome('outcome probe c', { fetchImpl: hit }); + assert.equal(found.answered, true); + assert.equal(found.place.name, 'Hà Nội'); + + // A memo hit is by definition an answer — only answers are stored. + assert.deepEqual( + await geocodeKeylessWithOutcome('outcome probe a', { fetchImpl: failure }), + { place: null, answered: true }, + ); +}); + +test('a biased pass that never answered leaves the whole lookup unanswered', async () => { + // The unbiased retry can report an honest empty while the biased request that + // preceded it timed out. The lookup as a whole still rests on a request that + // never came back, so its null is not a verdict. + const urls = []; + const halfDown = (url) => { + urls.push(url); + return new URL(url).searchParams.has('lat') + ? Promise.reject(new Error('timeout')) + : Promise.resolve({ ok: true, json: () => Promise.resolve({ features: [] }) }); + }; + + const outcome = await geocodeKeylessWithOutcome('half-down probe', { + bias: '30.20,-97.80|30.30,-97.70', fetchImpl: halfDown, + }); + assert.deepEqual(outcome, { place: null, answered: false }); + assert.equal(urls.length, 2); +}); diff --git a/src/voice/gevActions.js b/src/voice/gevActions.js index a56c840e7..a64f4b44d 100644 --- a/src/voice/gevActions.js +++ b/src/voice/gevActions.js @@ -17,6 +17,7 @@ import militaryAwarenessLayer, { import { initCameraVerbs, moveCamera, flyRoute, interruptCameraMotion, adjustOrbitRange } from '../cameraVerbs.js'; import { cachedGroundFloor, warmGroundFloor } from '../data/groundFloor.js'; import { isPickedWorldPosition } from '../data/scenePick.js'; +import { geocodeKeyless } from '../keylessGeocoder.js'; import { resolveRegionRingForQuery } from '../annotations/annotationResolver.js'; import { normalizeRadioCountryInput } from '../data/radioCountry.js'; import { TR3B_CLASS } from '../data/tr3bRegistry.js'; @@ -1265,25 +1266,43 @@ async function resolveRadioLocation(args = {}, coordinates = radioCoordinatePair if (known) return known; if (!query) return null; const apiKey = window.__GOOGLE_MAPS_API_KEY__ || import.meta.env.GOOGLE_MAPS_API_KEY; - if (!apiKey) throw new Error('No Google Maps API key available for Radio location search'); const controller = new AbortController(); const cancelFromTurn = () => controller.abort(); if (options.signal?.aborted) throw radioAbortError(); options.signal?.addEventListener('abort', cancelFromTurn, { once: true }); const timer = setTimeout(() => controller.abort(), 6000); try { - const url = `https://maps.googleapis.com/maps/api/geocode/json?address=${encodeURIComponent(query)}&key=${apiKey}`; - const response = await fetch(url, { signal: controller.signal }); - const body = await response.json(); + if (apiKey) { + const url = `https://maps.googleapis.com/maps/api/geocode/json?address=${encodeURIComponent(query)}&key=${apiKey}`; + const response = await fetch(url, { signal: controller.signal }); + const body = await response.json(); + if (!radioActionIsCurrent(options)) throw radioAbortError(); + const result = body.status === 'OK' ? body.results?.[0] : null; + if (result?.geometry?.location) { + return { + lat: result.geometry.location.lat, + lon: result.geometry.location.lng, + label: result.formatted_address || query, + country: '', + }; + } + } + // Same fallback rule as the LOCATION search box: reached with no key, and + // also when a key exists but Google returned nothing. A missing key used to + // throw here, which surfaced as a failed voice turn rather than as a radio + // station that could not be placed. + // Unbiased, matching the Google call above: a radio location is named + // outright ("radio near Hanoi"), not relative to what the camera shows. + const keyless = await geocodeKeyless(query); if (!radioActionIsCurrent(options)) throw radioAbortError(); - const result = body.status === 'OK' ? body.results?.[0] : null; - if (!result?.geometry?.location) return null; - return { - lat: result.geometry.location.lat, - lon: result.geometry.location.lng, - label: result.formatted_address || query, - country: '', - }; + if (!keyless) return null; + // `country` stays empty, exactly as the Google branch above leaves it. Photon + // does report a country, but in the feature's own language ("Việt Nam", + // "日本"), and `normalizeRadioCountryInput` fails CLOSED on a name it cannot + // map — so forwarding it would turn "radio near Ha Long" into an empty + // station list rather than the nearest station. A keyless lookup must not + // apply a filter the keyed one never applies. + return { lat: keyless.lat, lon: keyless.lng, label: keyless.label || query, country: '' }; } finally { clearTimeout(timer); options.signal?.removeEventListener('abort', cancelFromTurn); diff --git a/src/voice/gevActions.test.mjs b/src/voice/gevActions.test.mjs index 6d61e2d4d..a1bd44688 100644 --- a/src/voice/gevActions.test.mjs +++ b/src/voice/gevActions.test.mjs @@ -6,6 +6,7 @@ import { getContextStore, registerEntityContext } from '../data/contextStore.js' import { DataLayerManager } from '../data/manager.js'; import { getActiveCameraMotion, interruptCameraMotion, moveCamera } from '../cameraVerbs.js'; import { reassertNavigationHandoff, runExplicitNavigation } from '../navigationPolicy.js'; +import { normalizeRadioCountryInput } from '../data/radioCountry.js'; import { TR3B_CLASS } from '../data/tr3bRegistry.js'; import { controlCctv, @@ -3020,3 +3021,120 @@ test('front5: 0.99 km due EAST is the subject, though a degree box rejects it', assert.equal(result.window.centeredOn, 'N546PC'); }); }); + +// ── Keyless Radio location ─────────────────────────────────────────────────── +// +// `resolveRadioLocation` used to be Google-only: no key threw, and a key that +// geocoded to nothing returned null, which the caller reports as "Could not +// resolve Radio location". Both now fall through to Photon. These drive the +// second case, because it reaches the SAME fallback through a running Google +// branch — the no-key branch cannot be driven here, since the key expression +// reads `import.meta.env`, which only Vite defines. + +/** Photon's GeoJSON shape, trimmed to the properties the adapter consumes. */ +function photonFeature({ name, lat, lon, city = '', country = '' }) { + return { + geometry: { type: 'Point', coordinates: [lon, lat] }, + properties: { name, city, country, osm_key: 'place', osm_value: 'city' }, + }; +} + +/** A Radio layer that records what it was asked to select. */ +function radioSelectionHarness() { + let enabled = false; + const calls = []; + const state = { + stationCount: 4, filter: 'all', selected: null, + audioState: 'stopped', volume: 0.8, voiceDucked: false, + }; + const radio = { + getUIState: () => ({ ...state }), + selectRequestedStation(criteria, options) { + calls.push({ criteria, options }); + state.selected = { id: 'kl-1', name: 'Keyless FM' }; + return state.selected; + }, + }; + return { + calls, + dataManager: { + layers: new Map([['radio', { module: radio }]]), + isEnabled: () => enabled, + async setEnabled(_id, value) { enabled = value; }, + }, + }; +} + +/** Install a Google key plus a fetch stub, restoring both afterwards. */ +function installKeyedFetch(t, handler) { + globalThis.window = globalThis.window || { clearTimeout, setTimeout, requestIdleCallback: null }; + const priorKey = globalThis.window.__GOOGLE_MAPS_API_KEY__; + const priorFetch = globalThis.fetch; + globalThis.window.__GOOGLE_MAPS_API_KEY__ = 'unit-test-key'; + globalThis.fetch = handler; + t.after(() => { + globalThis.fetch = priorFetch; + if (priorKey === undefined) delete globalThis.window.__GOOGLE_MAPS_API_KEY__; + else globalThis.window.__GOOGLE_MAPS_API_KEY__ = priorKey; + }); +} + +test('voice Radio: a key that geocodes to nothing still places the station, keylessly', async (t) => { + const { calls, dataManager } = radioSelectionHarness(); + const requests = []; + installKeyedFetch(t, async (url) => { + requests.push(String(url)); + if (String(url).startsWith('https://maps.googleapis.com/')) { + return { ok: true, json: async () => ({ status: 'ZERO_RESULTS', results: [] }) }; + } + assert.match(String(url), /^https:\/\/photon\.komoot\.io\/api\/\?/); + return { + ok: true, + json: async () => ({ + features: [photonFeature({ + name: 'Hạ Long Bay', lat: 20.9101, lon: 107.1839, city: 'Hạ Long', country: 'Việt Nam', + })], + }), + }; + }); + + const result = await controlRadio({}, dataManager, { + action: 'select', locationQuery: 'Hạ Long Bay', + }); + + // Before the fallback existed this was `ok: false, "Could not resolve Radio location"`. + assert.equal(result.ok, true); + assert.equal(result.requestedLocation, 'Hạ Long Bay, Hạ Long, Việt Nam'); + assert.equal(calls.length, 1); + assert.ok(Math.abs(calls[0].criteria.anchor.lat - 20.9101) < 1e-9); + assert.ok(Math.abs(calls[0].criteria.anchor.lon - 107.1839) < 1e-9); + // Google is asked first and exactly once; Photon answers unbiased, in one call. + assert.equal(requests.filter((url) => url.includes('maps.googleapis.com')).length, 1); + assert.equal(requests.filter((url) => url.includes('photon.komoot.io')).length, 1); + assert.match(requests.at(-1), /[?&]q=H%E1%BA%A1\+Long\+Bay/); + assert.doesNotMatch(requests.at(-1), /[?&](lat|lon|bbox)=/, 'a named radio location is not viewport-biased'); +}); + +test('voice Radio: the keyless path applies no country filter the keyed path would not', async (t) => { + // Photon reports the country in the feature's own language ("Việt Nam"), and + // `rankRadioStationsForRequest` fails CLOSED on a country it cannot map — + // returning NO stations. Forwarding it would make a keyless install answer + // "No Radio station matched" for exactly the places it just resolved, while a + // keyed install placed a station. The label may carry it; the filter may not. + const { calls, dataManager } = radioSelectionHarness(); + installKeyedFetch(t, async (url) => (String(url).startsWith('https://maps.googleapis.com/') + ? { ok: true, json: async () => ({ status: 'ZERO_RESULTS', results: [] }) } + : { + ok: true, + json: async () => ({ + features: [photonFeature({ name: 'Kraków', lat: 50.0614, lon: 19.9366, country: 'Polska' })], + }), + })); + + const result = await controlRadio({}, dataManager, { action: 'select', locationQuery: 'Kraków' }); + + assert.equal(result.ok, true); + assert.equal(calls[0].criteria.country, '', 'a localized country name must never reach the station filter'); + assert.equal(normalizeRadioCountryInput('Polska').valid, false, 'and this is why: it would match nothing'); + assert.equal(result.requestedLocation, 'Kraków, Polska', 'the label still names the country honestly'); +});