From 10f1986e38eec0b031ec1f09eca810d216ccf655 Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Mon, 12 Oct 2026 13:52:39 +0100 Subject: [PATCH] fix(street-level): IC8 follow-ups for Google Street View - An open after a failed one is no longer failed by the old panorama's status: only status_changed can reject; position_changed only resolves. - A nearest lookup aborted, timed out or refused while Google's script loads no longer sends its getPanorama request once the script arrives. - A panorama-details lookup that never answers times out after 10 s, cools down and retries; an unmount releases lookups in flight, and their late answers are dropped. - Every Maps loader on a page shares one script load and ready callback, and either can retry a failed script. - OPEN STREET VIEW aimed at the sky opens nothing and says to aim at a street, instead of looking up the camera footprint. Co-Authored-By: Claude Opus 5.5 --- src/layers/streetLevel/index.js | 28 +++++-- src/layers/streetLevel/index.test.mjs | 41 ++++++++++ src/layers/streetLevel/policy.js | 4 + .../streetLevel/providers/google/index.js | 6 ++ .../providers/google/mapsLoader.js | 19 +++-- .../providers/google/mapsLoader.test.mjs | 31 ++++++++ .../streetLevel/providers/google/policy.js | 2 + .../providers/google/provider.test.mjs | 40 ++++++++++ .../streetLevel/providers/google/viewer.js | 62 +++++++++++---- .../providers/google/viewer.test.mjs | 76 ++++++++++++++++++- 10 files changed, 279 insertions(+), 30 deletions(-) diff --git a/src/layers/streetLevel/index.js b/src/layers/streetLevel/index.js index 07ea58d32..8d908dfff 100644 --- a/src/layers/streetLevel/index.js +++ b/src/layers/streetLevel/index.js @@ -22,6 +22,7 @@ import { createMeshSampler } from './meshSampler.js'; /** Above this camera height the ground under the camera is not worth fetching. */ const SURFACE_WARM_BELOW_M = 6000; import { + AIM_AT_GROUND_MESSAGE, FOLLOW_MAP_STACK_ID, NEAREST_RADIUS_M, POSITION_PICK_ID, @@ -298,11 +299,15 @@ export function createStreetLevelLayer({ nearestLookup = null; } - /** The ground under the middle of the screen, or null (no scenePick, sky). */ + /** + * The ground under the middle of the screen; null when the middle is sky; + * undefined when nothing can aim (no scenePick, or a canvas with no size). + */ function screenCentreGround() { const canvas = state.viewer?.scene?.canvas; const pick = state.services.scenePick?.pickGroundPosition; - if (!pick || !canvas?.clientWidth || !canvas?.clientHeight) return null; + if (!pick || !canvas?.clientWidth || !canvas?.clientHeight) + return undefined; const position = pick(state.viewer, { x: canvas.clientWidth / 2, y: canvas.clientHeight / 2, @@ -608,16 +613,25 @@ export function createStreetLevelLayer({ /** * Open the nearest image any active provider (or only `providerIds`) * has around a point; without one, around the view centre, or with - * `aim: 'screen-centre'` the ground under the middle of the screen. + * `aim: 'screen-centre'` the ground under the middle of the screen. An + * aim at the sky opens nothing rather than somewhere else. */ async openNearest( point, { providerIds = null, frame = true, aim = 'view' } = {}, ) { - const view = - point || - (aim === 'screen-centre' && screenCentreGround()) || - viewCentre(state.viewer); + let view = point; + if (!view && aim === 'screen-centre') { + view = screenCentreGround(); + if (view === null) { + abortNearest(); + state.street.loading = false; + state.street.error = AIM_AT_GROUND_MESSAGE; + notify(); + return false; + } + } + view ||= viewCentre(state.viewer); if (!Number.isFinite(view?.lat) || !Number.isFinite(view?.lon)) return false; abortNearest(); diff --git a/src/layers/streetLevel/index.test.mjs b/src/layers/streetLevel/index.test.mjs index 890a3c0ca..042aee012 100644 --- a/src/layers/streetLevel/index.test.mjs +++ b/src/layers/streetLevel/index.test.mjs @@ -649,6 +649,47 @@ test('the button’s “centre of the view” is the ground under the middle of assert.ok(Math.abs(lookups[0].lon - -121.4937) < 1e-9); }); +test('the button aimed at the sky opens nothing, and says to aim at a street', async (t) => { + const { createStreetLevelLayer: create } = await import('./index.js'); + const scenePick = await import('../../data/scenePick.js'); + const lookups = []; + const provider = fakeProvider({ + nearestImage: async (point) => { + lookups.push(point); + return null; + }, + }); + const saved = globalThis.document; + const drawing = new Proxy({}, { get: () => () => ({ addColorStop() {} }) }); + globalThis.document = Object.assign(new EventTarget(), { + createElement: () => ({ getContext: () => drawing }), + }); + const viewer = fakeViewer(); + // Every ground pick misses: the middle of the screen is sky. + Object.assign(viewer.scene, { + pickPositionSupported: true, + pickPosition: () => undefined, + }); + viewer.camera.pickEllipsoid = () => undefined; + viewer.scene.canvas.clientWidth = 1000; + viewer.scene.canvas.clientHeight = 600; + const layer = create({ providers: [provider], services: { scenePick } }); + layer.init(viewer); + layer.enable(viewer); + t.after(() => { + layer.destroy(); + globalThis.document = saved; + }); + assert.equal( + await layer.openNearest(undefined, { aim: 'screen-centre' }), + false, + ); + assert.deepEqual(lookups, [], 'no lookup at the view centre instead'); + const { street } = layer.getUIState(); + assert.match(street.error, /Aim the middle of the view at a street/); + assert.equal(street.loading, false); +}); + test('with no Mapillary token, the layer points at Street View, on Google 3D only', async (t) => { const { layer, stack } = await withStreetView(t, { mapillaryKeyless: true }); await settle(); diff --git a/src/layers/streetLevel/policy.js b/src/layers/streetLevel/policy.js index 391d464e4..4a25d9c6d 100644 --- a/src/layers/streetLevel/policy.js +++ b/src/layers/streetLevel/policy.js @@ -42,3 +42,7 @@ export const FOLLOW_EYE_HEIGHT_M = 2.4; * (Google Street View) only from this close: from higher up it is a misclick. */ export const GROUND_CLICK_MAX_HEIGHT_M = 3_000; + +/** OPEN STREET VIEW aimed at the sky: it opens what the user aims at, or nothing. */ +export const AIM_AT_GROUND_MESSAGE = + 'Aim the middle of the view at a street to look for imagery there'; diff --git a/src/layers/streetLevel/providers/google/index.js b/src/layers/streetLevel/providers/google/index.js index 7ac750801..b76a30be7 100644 --- a/src/layers/streetLevel/providers/google/index.js +++ b/src/layers/streetLevel/providers/google/index.js @@ -155,9 +155,13 @@ export function createGoogleProvider({ signal?.throwIfAborted(); const filter = context.getFilter(); if (filter.pano === 'flat') return null; + // Set once the caller has its answer (aborted, timed out, refused): + // a lookup still waiting on Google's script then asks nothing. + let settled = false; const lookup = (async () => { const lib = await loader.importLibrary('streetView'); await getService(); + if (settled) return null; const { GOOGLE, OUTDOOR } = lib.StreetViewSource; try { const { data } = await service.getPanorama({ @@ -177,6 +181,8 @@ export function createGoogleProvider({ ms: NEAREST_TIMEOUT_MS, signal, loader, + }).finally(() => { + settled = true; }); const panoId = data?.location?.pano; if (!panoId) return null; diff --git a/src/layers/streetLevel/providers/google/mapsLoader.js b/src/layers/streetLevel/providers/google/mapsLoader.js index 8b338e40b..1d9f0555d 100644 --- a/src/layers/streetLevel/providers/google/mapsLoader.js +++ b/src/layers/streetLevel/providers/google/mapsLoader.js @@ -7,6 +7,11 @@ const SCRIPT_ORIGIN = 'https://maps.googleapis.com'; const READY_CALLBACK = '__gevGoogleMapsReady'; +/** + * The page's one script load, kept on the page: every loader on it (two + * application instances, say) waits on the same script and callback. + */ +const PAGE_LOAD = Symbol.for('gev.googleMapsLoad'); /** * @param {{getApiKey: () => string|null|undefined, document?: Document, global?: object}} options @@ -16,8 +21,6 @@ export function createMapsLoader({ document = globalThis.document, global = globalThis, }) { - /** The script load in flight or done; cleared when it fails, so a retry can. */ - let loading = null; let authFailed = false; let authHandlerInstalled = false; const authListeners = new Set(); @@ -41,14 +44,15 @@ export function createMapsLoader({ } function load() { - if (loading) return loading; const key = getApiKey?.(); if (!key) return Promise.reject(new Error('GOOGLE_MAPS_API_KEY is not set')); - if (global.google?.maps?.importLibrary) - return (loading = Promise.resolve(global.google.maps)); installAuthHandler(); - loading = new Promise((resolve, reject) => { + if (global[PAGE_LOAD]) return global[PAGE_LOAD]; + if (global.google?.maps?.importLibrary) + return Promise.resolve(global.google.maps); + // Cleared when the script fails, so a retry (from any loader) can. + const loading = new Promise((resolve, reject) => { global[READY_CALLBACK] = () => { delete global[READY_CALLBACK]; resolve(global.google.maps); @@ -62,13 +66,14 @@ export function createMapsLoader({ callback: READY_CALLBACK, })}`; script.onerror = () => { - loading = null; + if (global[PAGE_LOAD] === loading) delete global[PAGE_LOAD]; script.remove(); delete global[READY_CALLBACK]; reject(new Error('The Google Maps JavaScript API could not load')); }; document.head.append(script); }); + global[PAGE_LOAD] = loading; return loading; } diff --git a/src/layers/streetLevel/providers/google/mapsLoader.test.mjs b/src/layers/streetLevel/providers/google/mapsLoader.test.mjs index ac7f8fe91..789353143 100644 --- a/src/layers/streetLevel/providers/google/mapsLoader.test.mjs +++ b/src/layers/streetLevel/providers/google/mapsLoader.test.mjs @@ -53,6 +53,37 @@ test('the API loads once, with the key, asynchronously, and hands out libraries' assert.equal(page.global[callback], undefined, 'the callback is cleaned up'); }); +test('two loaders on one page share one script and both get the API', async () => { + const page = fakePage(); + const first = createMapsLoader({ getApiKey: () => 'k', ...page }); + const second = createMapsLoader({ getApiKey: () => 'k', ...page }); + const one = first.importLibrary('streetView'); + const two = second.importLibrary('streetView'); + await settle(); + assert.equal(page.scripts.length, 1, 'one script for the page'); + page.loadScript(); + assert.equal((await one).name, 'streetView'); + assert.equal((await two).name, 'streetView'); + assert.equal((await first.importLibrary('streetView')).name, 'streetView'); +}); + +test('after a failed script, either loader on the page can retry it', async () => { + const page = fakePage(); + const first = createMapsLoader({ getApiKey: () => 'k', ...page }); + const second = createMapsLoader({ getApiKey: () => 'k', ...page }); + const failed = first.importLibrary('streetView'); + await settle(); + page.scripts[0].onerror(); + await assert.rejects(failed, /could not load/); + const retry = second.importLibrary('streetView'); + const again = first.importLibrary('streetView'); + await settle(); + assert.equal(page.scripts.length, 1); + page.loadScript(); + assert.equal((await retry).name, 'streetView'); + assert.equal((await again).name, 'streetView'); +}); + test('without a key nothing loads', async () => { const page = fakePage(); const loader = createMapsLoader({ getApiKey: () => null, ...page }); diff --git a/src/layers/streetLevel/providers/google/policy.js b/src/layers/streetLevel/providers/google/policy.js index 8d76d4bfa..ea4ba8078 100644 --- a/src/layers/streetLevel/providers/google/policy.js +++ b/src/layers/streetLevel/providers/google/policy.js @@ -31,6 +31,8 @@ export const KEY_REJECTED_MESSAGE = export const NEAREST_TIMEOUT_MS = 10_000; /** A panorama that has not shown by then has failed to open. */ export const OPEN_TIMEOUT_MS = 20_000; +/** A panorama-details lookup that takes longer has failed (and can retry). */ +export const DETAILS_TIMEOUT_MS = 10_000; /** Minimum pause after a failed panorama-details lookup before a view can retry. */ export const DETAILS_RETRY_COOLDOWN_MS = 5_000; diff --git a/src/layers/streetLevel/providers/google/provider.test.mjs b/src/layers/streetLevel/providers/google/provider.test.mjs index 874e3a94f..71d45072a 100644 --- a/src/layers/streetLevel/providers/google/provider.test.mjs +++ b/src/layers/streetLevel/providers/google/provider.test.mjs @@ -206,6 +206,46 @@ test('aborting a lookup in flight gives up at once', async () => { await assert.rejects(lookup, { name: 'AbortError' }); }); +test('a lookup cancelled while Google\u2019s script loads asks Google nothing', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout'] }); + const { NEAREST_TIMEOUT_MS } = await import('./policy.js'); + const fake = fakeStreetViewLibrary({ + panoramas: PANORAMAS, + nearest: () => 'fresh', + }); + const loader = fakeMapsLoader(fake.library, { deferred: true }); + const instance = createGoogleProvider({ + getApiKey: () => 'browser-key', + loader, + }).create({ + services: {}, + getFilter: () => ({ pano: 'all', sinceMs: null }), + isActive: () => true, + notify() {}, + actions: { reportError() {} }, + }); + const flush = async () => { + for (let i = 0; i < 10; i++) await Promise.resolve(); + }; + // Aborted, then timed out, while the script still loads. + const controller = new AbortController(); + const aborted = instance.nearestImage( + { lat: 0, lon: 0 }, + { signal: controller.signal }, + ); + controller.abort(); + await assert.rejects(aborted, { name: 'AbortError' }); + const expired = instance.nearestImage({ lat: 0, lon: 0 }); + t.mock.timers.tick(NEAREST_TIMEOUT_MS); + await assert.rejects(expired, /did not answer/); + // The script arrives: a current lookup still gets its answer. + const current = instance.nearestImage({ lat: 0, lon: 0 }); + loader.release(); + await flush(); + assert.equal(await current, 'fresh'); + assert.equal(fake.lookups.length, 1, 'only the current lookup asked Google'); +}); + test('a Google outage is an error, not "no imagery here"', async () => { const { instance, fake } = setup(); fake.library.StreetViewService.prototype.getPanorama = () => diff --git a/src/layers/streetLevel/providers/google/viewer.js b/src/layers/streetLevel/providers/google/viewer.js index 27fd8cb1e..c04a3acbd 100644 --- a/src/layers/streetLevel/providers/google/viewer.js +++ b/src/layers/streetLevel/providers/google/viewer.js @@ -1,5 +1,6 @@ import { DETAILS_RETRY_COOLDOWN_MS, + DETAILS_TIMEOUT_MS, GOOGLE_PROVIDER_ID, KEY_REJECTED_MESSAGE, NO_ANSWER_MESSAGE, @@ -49,8 +50,11 @@ export function createGoogleViewer({ loader, getService, render } = {}) { let subscriptions = []; /** Panorama id → its {capturedAt, creator, title}. */ const described = new Map(); - /** Panorama ids whose lookup is in flight. */ - const lookingUp = new Set(); + /** + * Panorama id → its lookup in flight ({timer}). A late answer to a lookup + * no longer here (timed out, or unmounted) is dropped. + */ + const lookingUp = new Map(); /** Panorama id → earliest retry time after a failed details lookup. */ const retryAfter = new Map(); const listeners = new Set(); @@ -84,29 +88,49 @@ export function createGoogleViewer({ loader, getService, render } = {}) { /** * Look up a panorama's date, photographer and address once; the pose - * already out is sent again with them. After a failure, view events can - * retry once the cooldown expires. + * already out is sent again with them. A lookup that fails or takes longer + * than DETAILS_TIMEOUT_MS lets view events retry once the cooldown expires. */ async function describe(panoId) { if (described.has(panoId) || lookingUp.has(panoId)) return; if (Date.now() < (retryAfter.get(panoId) ?? 0)) return; - lookingUp.add(panoId); + const lookup = { timer: null }; + lookingUp.set(panoId, lookup); + const current = () => lookingUp.get(panoId) === lookup; try { - service ||= getService - ? await getService() - : new (await ensureLibrary()).StreetViewService(); - const { data } = await service.getPanorama({ pano: panoId }); + const { data } = await new Promise((resolve, reject) => { + lookup.timer = setTimeout( + () => reject(new Error(NO_ANSWER_MESSAGE)), + DETAILS_TIMEOUT_MS, + ); + (async () => { + service ||= getService + ? await getService() + : new (await ensureLibrary()).StreetViewService(); + return service.getPanorama({ pano: panoId }); + })().then(resolve, reject); + }); + // Released by an unmount: the viewer it was for is gone. + if (!current()) return; described.set(panoId, metaFrom(data)); retryAfter.delete(panoId); } catch { - retryAfter.set(panoId, Date.now() + DETAILS_RETRY_COOLDOWN_MS); + if (current()) + retryAfter.set(panoId, Date.now() + DETAILS_RETRY_COOLDOWN_MS); return; } finally { - lookingUp.delete(panoId); + clearTimeout(lookup.timer); + if (current()) lookingUp.delete(panoId); } publishPose(); } + /** Drop every lookup in flight: a remount looks again. */ + function releaseLookups() { + for (const lookup of lookingUp.values()) clearTimeout(lookup.timer); + lookingUp.clear(); + } + /** Emit the pose on screen now, dated once its lookup has answered. */ function publishPose() { const panoId = panorama?.getPano?.(); @@ -189,7 +213,9 @@ export function createGoogleViewer({ loader, getService, render } = {}) { finish(reject, new Error(KEY_REJECTED_MESSAGE)), ); cancelShown = () => finish(resolve); - const check = () => { + // Only a status event can fail the open: at a position event the + // status may still be the previous panorama's failure. + const onStatus = () => { if (pendingOpen !== panoId) return finish(resolve); const status = instance.getStatus(); if (status === 'OK') finish(resolve); @@ -199,9 +225,14 @@ export function createGoogleViewer({ loader, getService, render } = {}) { new Error('Street View has no imagery for this place'), ); }; - const subscriptions = ['status_changed', 'position_changed'].map((name) => - instance.addListener(name, check), - ); + const onPosition = () => { + if (pendingOpen !== panoId || instance.getStatus() === 'OK') + finish(resolve); + }; + const subscriptions = [ + instance.addListener('status_changed', onStatus), + instance.addListener('position_changed', onPosition), + ]; function finish(settle, value) { cancelShown = null; clearTimeout(timer); @@ -247,6 +278,7 @@ export function createGoogleViewer({ loader, getService, render } = {}) { unmount() { pendingOpen = null; cancelShown?.(); + releaseLookups(); generation++; destroyPanorama(); }, diff --git a/src/layers/streetLevel/providers/google/viewer.test.mjs b/src/layers/streetLevel/providers/google/viewer.test.mjs index 32602575e..a3d746a34 100644 --- a/src/layers/streetLevel/providers/google/viewer.test.mjs +++ b/src/layers/streetLevel/providers/google/viewer.test.mjs @@ -1,7 +1,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { createGoogleViewer } from './viewer.js'; -import { DETAILS_RETRY_COOLDOWN_MS } from './policy.js'; +import { DETAILS_RETRY_COOLDOWN_MS, DETAILS_TIMEOUT_MS } from './policy.js'; import { fakeHost, fakeMapsLoader, @@ -48,6 +48,7 @@ test('opening a panorama shows it and emits a provider-neutral pose', async () = await viewer.open('pano-a'); assert.equal(panorama.visible, true); + await settle(); // the pose is dated once its lookup answers const pose = poses.at(-1); assert.deepEqual( { @@ -207,6 +208,7 @@ test('an open never waits on the date lookup', async () => { new Promise(() => {}); // the metadata never answers await viewer.open('pano-a'); assert.equal(poses.at(-1).imageId, 'pano-a'); + viewer.unmount(); }); test('closing while a panorama loads settles the open and drops its listeners', async () => { @@ -329,3 +331,75 @@ test('a failed panorama does not delay details for another panorama', async (t) 'returning respects the pause', ); }); + +test('a retry after a failed open is not failed by the old panorama\u2019s status', async () => { + const { viewer, poses, host } = setup(); + await viewer.mount(host); + await assert.rejects(viewer.open('gone'), /no imagery/); + // The new panorama moves first; its OK status follows. + await viewer.open('pano-a'); + assert.equal(poses.at(-1).imageId, 'pano-a'); +}); + +test('a details lookup that never answers times out, cools down and retries', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout', 'Date'], now: 1_000 }); + const flush = async () => { + for (let i = 0; i < 10; i++) await Promise.resolve(); + }; + const { fake, viewer, poses, host } = setup(); + await viewer.mount(host); + const getPanorama = fake.library.StreetViewService.prototype.getPanorama; + const requests = []; + let hang = true; + fake.library.StreetViewService.prototype.getPanorama = function (request) { + requests.push(request.pano); + return hang ? new Promise(() => {}) : getPanorama.call(this, request); + }; + const opening = viewer.open('pano-a'); + await flush(); + await opening; + assert.deepEqual(requests, ['pano-a']); + fake.built[0].setPov({ heading: 5, pitch: 0 }); + await flush(); + assert.deepEqual(requests, ['pano-a'], 'one lookup in flight at a time'); + t.mock.timers.tick(DETAILS_TIMEOUT_MS); + await flush(); + hang = false; + fake.built[0].setPov({ heading: 6, pitch: 0 }); + await flush(); + assert.deepEqual(requests, ['pano-a'], 'the cooldown follows a timeout'); + t.mock.timers.tick(DETAILS_RETRY_COOLDOWN_MS); + fake.built[0].setPov({ heading: 7, pitch: 0 }); + await flush(); + assert.deepEqual(requests, ['pano-a', 'pano-a']); + assert.equal(poses.at(-1).capturedAt, Date.UTC(2024, 4, 1)); + assert.equal(poses.at(-1).bearing, 7); +}); + +test('a remount looks up again a panorama whose lookup hung', async () => { + const { fake, viewer, poses, host } = setup(); + await viewer.mount(host); + const getPanorama = fake.library.StreetViewService.prototype.getPanorama; + const requests = []; + let answerHung; + fake.library.StreetViewService.prototype.getPanorama = function (request) { + requests.push(request.pano); + if (requests.length === 1) + return new Promise((resolve) => { + answerHung = () => resolve(getPanorama.call(this, request)); + }); + return getPanorama.call(this, request); + }; + await viewer.open('pano-b'); + viewer.unmount(); + await viewer.mount(host); + await viewer.open('pano-b'); + await settle(); + assert.deepEqual(requests, ['pano-b', 'pano-b']); + assert.equal(poses.at(-1).creator, 'Ada'); + // The hung answer arriving now is for a viewer that is gone: dropped. + const count = poses.length; + answerHung(); + await settle(); + assert.equal(poses.length, count); +});