From 552f3de3ff764422f7a0dcc491a1b94ac42405ad Mon Sep 17 00:00:00 2001 From: Manjunath Date: Tue, 3 Nov 2026 17:55:33 +0530 Subject: [PATCH] fix(voice): resolve area names to the place they name Three resolver fixes from checking the review focus: - A natural region named like a country or state no longer hides it. "India" was the peninsula, "Japan" an island group, "Florida" the peninsula and "Punjab" a desert, so bare "Punjab" never asked which one. The region still answers when asked for by level, and when no unit truly has the name ("Great Britain" finds the United Kingdom only by alias). - Same-named units get their own candidate: "Washington" offered WA and DC with one id, so either choice was DC and a DC lookup overwrote a stored Washington State. Ids now carry the region code, and choices show it. - A bundled unit that only shares the name ("Azad Kashmir" for "Kashmir") no longer ends the search; the outline ladder still looks, and the mismatch is kept if it finds nothing. Retired zones stay refused. --- src/data/areaResolver.js | 49 +++++++++++---- src/data/areaResolver.test.mjs | 102 +++++++++++++++++++++++++++++++ src/voice/actionSchemas.test.mjs | 2 +- src/voice/resultDisplay.js | 4 +- 4 files changed, 143 insertions(+), 14 deletions(-) diff --git a/src/data/areaResolver.js b/src/data/areaResolver.js index 2d483cf80..0cbcd4894 100644 --- a/src/data/areaResolver.js +++ b/src/data/areaResolver.js @@ -150,10 +150,11 @@ function stableCandidateHash(value) { function candidateIdOf(admin) { const prefix = `ne:${admin.kind}:`; - const slug = normalizeAreaQuery(`${admin.country} ${admin.name}`).replace( - / /g, - '-', - ); + // The region code keeps same-named units apart: "Washington" is both WA + // and DC, which would otherwise share one candidate and one stored area. + const slug = normalizeAreaQuery( + `${admin.country} ${admin.name} ${admin.regionCode || ''}`, + ).replace(/ /g, '-'); const full = `${prefix}${slug}`; if (full.length <= MAX_AREA_CANDIDATE_ID_LENGTH) return full; const suffix = `-${stableCandidateHash(slug)}`; @@ -175,6 +176,10 @@ function describeCandidate(admin) { candidateId: candidateIdOf(admin), name: admin.name, level: ADMIN_LEVEL[admin.kind] || null, + // Tells same-named units apart when spoken: Washington WA vs DC. + ...(admin.regionCode && admin.kind !== 'country' + ? { region: String(admin.regionCode) } + : {}), ...(admin.country && admin.kind !== 'country' ? { country: admin.country } : {}), @@ -275,12 +280,24 @@ export function createAreaResolver({ async function lookup({ query, level, within }, { signal }) { const { level: levelHint } = splitLevelWord(query); const searchText = within ? `${query}, ${within}` : query; + let bundledRefusal = null; // Offline first: a named natural region, unless an admin level or a // country/state qualifier was given (the pack cannot check `within`). if ((!level || level === 'natural') && !within) { const ne = await findNaturalRegion(query, { signal }).catch(() => null); signal?.throwIfAborted?.(); - if (ne?.polygons?.length) { + // A region named like a country or state ("India" the peninsula, + // "Punjab" the desert) never hides it; asked for by level, it answers. + // Only a unit that really has this name counts: "Great Britain" finds + // the United Kingdom by alias, which is not a Great Britain. + const shadowsAdmin = + ne?.polygons?.length && + level !== 'natural' && + (await findAdminCandidates(query, { signal }).catch(() => [])).some( + (admin) => !bundledAdminIdentityRefusal(admin, query, null), + ); + signal?.throwIfAborted?.(); + if (ne?.polygons?.length && !shadowsAdmin) { const record = store.put({ geometry: ne.polygons.map((ring) => [ring]), name: ne.name, @@ -314,8 +331,14 @@ export function createAreaResolver({ places = checked .filter(({ refused }) => !refused) .map(({ admin }) => admin); - if (!places.length && checked.length) - return checked.find(({ refused }) => refused)?.refused; + if (!places.length && checked.length) { + const refused = checked.find((entry) => entry.refused)?.refused; + // A retired or unverifiable unit is final. A different unit that + // merely shares the name ("Azad Kashmir" for "Kashmir") is not an + // answer, so the outline ladder still gets to look. + if (refused?.code !== 'AREA_IDENTITY_MISMATCH') return refused; + bundledRefusal = refused; + } if (places.length === 1) { return storeAdmin(places[0], 0.95, identity); } @@ -354,11 +377,13 @@ export function createAreaResolver({ }); signal?.throwIfAborted?.(); if (!outline?.ok) - return { - ok: false, - code: outline?.code || 'AREA_NOT_FOUND', - error: outline?.error || `No boundary found for "${query}".`, - }; + return ( + bundledRefusal || { + ok: false, + code: outline?.code || 'AREA_NOT_FOUND', + error: outline?.error || `No boundary found for "${query}".`, + } + ); const record = store.put({ geometry: outline.polygons, name: outline.name || query, diff --git a/src/data/areaResolver.test.mjs b/src/data/areaResolver.test.mjs index f27dd8fbd..2e371ca87 100644 --- a/src/data/areaResolver.test.mjs +++ b/src/data/areaResolver.test.mjs @@ -623,3 +623,105 @@ test('a name that normalizes to nothing is never cached or shared', async () => await resolver.resolve({ query: '???' }); assert.equal(calls, 2, 'two punctuation-only asks are two lookups'); }); + +test('a natural region never hides a country or state of the same name', async () => { + const desert = { + name: 'Punjab', + kind: 'natural', + polygons: [square(70, 28, 76, 33)], + }; + const { resolver } = makeResolver({ + findNaturalRegion: async (q) => (/^punjab$/i.test(q) ? desert : null), + }); + const bare = await resolver.resolve({ query: 'Punjab' }); + assert.equal(bare.ok, false); + assert.equal(bare.code, 'AMBIGUOUS_AREA', 'the two states are asked about'); + const natural = await resolver.resolve({ query: 'Punjab', level: 'natural' }); + assert.equal(natural.ok, true); + assert.equal(natural.record.level, 'natural', 'asked for, the region wins'); +}); + +test('same-named states in one country get their own candidate and area', async () => { + const wa = admin( + 'state', + 'Washington', + 'United States of America', + [-124, 45.5, -117, 49], + { regionCode: 'WA' }, + ); + const dc = admin( + 'state', + 'Washington', + 'United States of America', + [-77.2, 38.8, -76.9, 39], + { regionCode: 'DC' }, + ); + const { resolver } = makeResolver({ + findAdminCandidates: async (q) => (/^washington$/i.test(q) ? [wa, dc] : []), + }); + const asked = await resolver.resolve({ query: 'Washington' }); + assert.equal(asked.code, 'AMBIGUOUS_AREA'); + const ids = asked.candidates.map((c) => c.candidateId); + assert.equal(new Set(ids).size, 2, `distinct ids: ${ids.join(', ')}`); + assert.deepEqual( + asked.candidates.map((c) => c.region), + ['WA', 'DC'], + 'the choices can be told apart', + ); + const picked = await Promise.all( + ids.map((candidateId) => resolver.resolve({ candidateId })), + ); + assert.ok(picked.every((p) => p.ok)); + assert.notEqual( + picked[0].record.areaId, + picked[1].record.areaId, + 'two areas, not one overwritten', + ); + assert.equal( + pointInPreparedArea(picked[0].record.prepared, 47, -120), + true, + 'Washington State', + ); + assert.equal( + pointInPreparedArea(picked[1].record.prepared, 38.9, -77), + true, + 'District of Columbia', + ); +}); + +test('a bundled name mismatch falls through to the outline ladder', async () => { + const azad = admin('state', 'Azad Kashmir', 'Pakistan', [73, 33, 75, 35.5]); + const kashmir = { + ok: true, + name: 'Kashmir', + polygons: [[square(73, 32, 80, 37)]], + source: 'osm', + rung: 'nominatim', + }; + let outlineOk = true; + const { resolver } = makeResolver({ + findAdminCandidates: async (q) => (/^kashmir$/i.test(q) ? [azad] : []), + resolveNamedOutline: async () => + outlineOk + ? kashmir + : { ok: false, code: 'AREA_NOT_FOUND', error: 'none' }, + }); + const found = await resolver.resolve({ query: 'Kashmir' }); + assert.equal(found.ok, true, found.error); + assert.equal(found.record.name, 'Kashmir'); + outlineOk = false; + const { resolver: offline } = makeResolver({ + findAdminCandidates: async () => [azad], + resolveNamedOutline: async () => ({ + ok: false, + code: 'AREA_NOT_FOUND', + error: 'none', + }), + }); + const refused = await offline.resolve({ query: 'Kashmir' }); + assert.equal( + refused.code, + 'AREA_IDENTITY_MISMATCH', + 'the bundled refusal is kept', + ); +}); diff --git a/src/voice/actionSchemas.test.mjs b/src/voice/actionSchemas.test.mjs index 6c12ba62a..165ed8737 100644 --- a/src/voice/actionSchemas.test.mjs +++ b/src/voice/actionSchemas.test.mjs @@ -103,7 +103,7 @@ test('every emitted area candidate round-trips through the production schema', a true, ); } - assert.equal(ambiguous.candidates[0].candidateId.length, 41); + assert.equal(ambiguous.candidates[0].candidateId.length, 44); const grammar = new RegExp(candidateSchema.pattern, 'u'); for (const malformed of [ 'ne:state:bad slug', diff --git a/src/voice/resultDisplay.js b/src/voice/resultDisplay.js index 5f4dd6006..3d44bf95e 100644 --- a/src/voice/resultDisplay.js +++ b/src/voice/resultDisplay.js @@ -80,7 +80,9 @@ function candidateLines(candidates) { ? t(LEVEL_WORDS[candidate.level]) : candidate.level : null; - const detail = [level, candidate?.country].filter(Boolean).join(', '); + const detail = [level, candidate?.region, candidate?.country] + .filter(Boolean) + .join(', '); return `${i + 1} · ${spokenLabel(candidate?.name || 'Unnamed', 40)}${detail ? ` — ${detail}` : ''}`; }); }