From a5e16e9470ecda0404989017479adf03fdf4c3bb Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Mon, 5 Oct 2026 12:44:27 +0100 Subject: [PATCH] fix(street-level): settle loading on pan, keep zoom 0, drop late poses Fixes from gekh's re-review of 061d269, each with a regression test that fails on the old code: - A pan that drops a tile still loading now settles the loading count (the aborted request no longer owns its key, so its `finally` never did), and purges the previous zoom's tiles once nothing is pending. LOADING no longer sticks on the status pill and chip. - Zoom 0 (above 15,000 km) is a zoom: `zoom == null` instead of `!zoom`. - A pose that arrives after a photo was closed is dropped, in the MapillaryJS adapter and in the viewer host, so the marker and sequence selection do not come back. - Ground casting passes the tile's signal to the terrain request, so a cancelled tile stops its height batch instead of holding the queue. - The server's in-memory tile cache honours the 24 h TTL like the disk. - The viewer's one-time resize runs for every photo, not just the first. Co-Authored-By: Claude Opus 5.5 --- server/providers/mapillary/tiles.js | 7 ++++ src/layers/streetLevel/groundCast.js | 1 + src/layers/streetLevel/groundCast.test.mjs | 16 +++++++++ .../providers/mapillary/coverage.js | 6 +++- .../providers/mapillary/coverage.test.mjs | 36 +++++++++++++++++++ .../streetLevel/providers/mapillary/viewer.js | 3 ++ src/layers/streetLevel/viewerHost.js | 2 +- src/layers/streetLevel/viewerHost.test.mjs | 23 ++++++++++-- src/tooling/mapillaryProvider.test.mjs | 18 ++++++++++ src/ui/streetLevelControls.js | 6 ++-- src/ui/streetLevelControls.test.mjs | 3 ++ 11 files changed, 115 insertions(+), 6 deletions(-) diff --git a/server/providers/mapillary/tiles.js b/server/providers/mapillary/tiles.js index 22d32baae..9768880a4 100644 --- a/server/providers/mapillary/tiles.js +++ b/server/providers/mapillary/tiles.js @@ -77,6 +77,13 @@ const _inFlight = new Map(); function memoryGet(key) { const hit = _memory.get(key); if (!hit) return null; + // Same 24 h life as the disk copy: a long-running server must not keep + // serving stale (or empty) coverage until eviction. + if (Date.now() - hit.at > TILE_DISK_TTL_MS) { + _memory.delete(key); + _memoryBytes -= hit.bytes.length; + return null; + } // Re-insert to mark as most recently used. _memory.delete(key); _memory.set(key, hit); diff --git a/src/layers/streetLevel/groundCast.js b/src/layers/streetLevel/groundCast.js index f3bfeea04..89dec4131 100644 --- a/src/layers/streetLevel/groundCast.js +++ b/src/layers/streetLevel/groundCast.js @@ -181,6 +181,7 @@ export function createGroundCaster({ lon: Number((i * step).toFixed(6)), lat: Number((j * step).toFixed(6)), })), + { signal }, ); let complete = true; missing.forEach(([key], n) => { diff --git a/src/layers/streetLevel/groundCast.test.mjs b/src/layers/streetLevel/groundCast.test.mjs index d1c35b221..8dd831781 100644 --- a/src/layers/streetLevel/groundCast.test.mjs +++ b/src/layers/streetLevel/groundCast.test.mjs @@ -238,3 +238,19 @@ test('a full cache is cleared before a request counts its corners, so the reques assert.equal(await caster.prepareLines([line]), true); assert.ok(caster.castLine(line), 'the whole line casts'); }); + +test('cancelling a prepare cancels its terrain request', async () => { + const seen = []; + const caster = createGroundCaster({ + terrain: { + async resolveEllipsoidalGround(coords, options) { + seen.push(options?.signal); + return coords.map(() => ({ ellipsoid: 100, source: 'reearth' })); + }, + }, + }); + const controller = new AbortController(); + await caster.prepare([[10.0005, 50.0005]], { signal: controller.signal }); + assert.equal(seen.length, 1); + assert.equal(seen[0], controller.signal, 'the tile signal reaches the proxy'); +}); diff --git a/src/layers/streetLevel/providers/mapillary/coverage.js b/src/layers/streetLevel/providers/mapillary/coverage.js index 783045bb0..7473c35a7 100644 --- a/src/layers/streetLevel/providers/mapillary/coverage.js +++ b/src/layers/streetLevel/providers/mapillary/coverage.js @@ -537,7 +537,7 @@ export function createCoverage({ state, source }) { let bbox = visibleBbox(viewer); if (kind === 'overview' && (!bbox || zoom <= 1)) bbox = [-180, -85, 180, 85]; - if (!zoom || !bbox) { + if (zoom == null || !bbox) { state.coverage.hint = 'Point the camera at the globe for street-level coverage'; clear(); @@ -559,10 +559,14 @@ export function createCoverage({ state, source }) { if (!wanted.has(key)) removeTile(key); for (const [key, controller] of [...state.coverage.pending]) if (!wanted.has(key)) { + // The aborted request no longer owns its key, so its `finally` will + // not settle the count: settle it here. controller.abort(); state.coverage.pending.delete(key); + state.coverage.loading = Math.max(0, state.coverage.loading - 1); } for (const tile of tiles) loadTile(tile, kind); + if (!state.coverage.pending.size) purgeStale(); // The camera moved: cells that were out of the sampler's range may not be. if (terrainMode()) for (const entry of state.coverage.tiles.values()) diff --git a/src/layers/streetLevel/providers/mapillary/coverage.test.mjs b/src/layers/streetLevel/providers/mapillary/coverage.test.mjs index 6f88028be..5c133cf6e 100644 --- a/src/layers/streetLevel/providers/mapillary/coverage.test.mjs +++ b/src/layers/streetLevel/providers/mapillary/coverage.test.mjs @@ -241,3 +241,39 @@ test('a tile whose lines cannot be cast is not rebuilt for nothing', async () => assert.equal(viewer.scene.primitives.items.size, 0); coverage.clear(); }); + +test('panning off a tile that is still loading settles the loading count', async () => { + const { viewer, source, state, coverage } = setup(); + coverage.refresh(); + assert.equal(state.coverage.loading, 1); + // Same zoom, two tiles east: the first request is dropped, a new one starts. + viewer.view.lon += 0.05; + coverage.refresh(); + assert.equal(source.calls.length, 2); + assert.equal(source.calls[0].signal.aborted, true); + assert.equal(state.coverage.pending.size, 1); + assert.equal( + state.coverage.loading, + 1, + 'only the live request counts as loading', + ); + source.calls[0].reject(new DOMException('aborted', 'AbortError')); + source.calls[1].resolve(new Uint8Array()); + await settle(); + assert.equal(state.coverage.pending.size, 0); + assert.equal(state.coverage.loading, 0, 'LOADING clears'); + coverage.clear(); +}); + +test('zoom 0 is a zoom: the whole-earth view still shows coverage', () => { + const { viewer, source, state, coverage } = setup(); + viewer.view.height = 20_000_000; + coverage.refresh(); + assert.equal(state.coverage.zoom, 0); + assert.equal(state.coverage.hint, ''); + assert.deepEqual( + source.calls.map((call) => call.key), + ['0/0/0'], + ); + coverage.clear(); +}); diff --git a/src/layers/streetLevel/providers/mapillary/viewer.js b/src/layers/streetLevel/providers/mapillary/viewer.js index 542a08888..f1da59980 100644 --- a/src/layers/streetLevel/providers/mapillary/viewer.js +++ b/src/layers/streetLevel/providers/mapillary/viewer.js @@ -69,6 +69,9 @@ export function createMapillaryViewer({ source, render } = {}) { viewer.getPosition(), viewer.getPointOfView(), ]); + // Closed (or never opened) while the pose was in flight: a late + // `image` event must not bring the photo back. + if (pendingOpen === null) return; if (image) current = describe(image); if (!current || !viewer) return; emit({ diff --git a/src/layers/streetLevel/viewerHost.js b/src/layers/streetLevel/viewerHost.js index 28768c344..b142951a0 100644 --- a/src/layers/streetLevel/viewerHost.js +++ b/src/layers/streetLevel/viewerHost.js @@ -25,7 +25,7 @@ export function createViewerHost({ state, parts }) { } function applyPose(pose) { - if (!active || pose.providerId !== active.id) return; + if (!active || !state.street.open || pose.providerId !== active.id) return; const previousSequence = state.street.sequenceId; Object.assign(state.street, { providerId: pose.providerId, diff --git a/src/layers/streetLevel/viewerHost.test.mjs b/src/layers/streetLevel/viewerHost.test.mjs index 018fc2025..4ef069e90 100644 --- a/src/layers/streetLevel/viewerHost.test.mjs +++ b/src/layers/streetLevel/viewerHost.test.mjs @@ -27,17 +27,22 @@ function fakeAdapter({ failMounts = 0 } = {}) { calls.mount++; if (calls.mount <= failMounts) throw new Error('library failed to load'); }, - async open(id) { - calls.open.push(id); + /** A pose event from the library, as MapillaryJS fires them. */ + emitPose(id) { emit?.({ providerId: 'mapillary', imageId: id, + sequenceId: `seq-${id}`, position: { lon: 1, lat: 2 }, bearing: 90, isPano: false, externalUrl: `https://example.test/${id}`, }); }, + async open(id) { + calls.open.push(id); + this.emitPose(id); + }, close() {}, unmount() { calls.unmount++; @@ -127,3 +132,17 @@ test('switching one provider off releases only its viewer', async () => { host.unmount('mapillary'); assert.equal(adapter.calls.unmount, 1); }); + +test('a pose that arrives after the photo was closed does not bring it back', async () => { + const adapter = fakeAdapter(); + const { state, host } = harness(adapter); + await host.open('mapillary', 'a'); + host.close(); + const before = { ...state.street }; + // The library's `image` event for a photo that was still loading. + adapter.emitPose('a'); + assert.equal(state.street.open, false); + assert.equal(state.street.imageId, null); + assert.equal(state.street.sequenceId, null); + assert.deepEqual(state.street, before); +}); diff --git a/src/tooling/mapillaryProvider.test.mjs b/src/tooling/mapillaryProvider.test.mjs index c601289b1..2c51386b3 100644 --- a/src/tooling/mapillaryProvider.test.mjs +++ b/src/tooling/mapillaryProvider.test.mjs @@ -202,6 +202,24 @@ test('fetchTile serves from memory after one upstream fetch and strips the image } }); +test('a tile held in memory past the 24 h TTL is fetched again', async (t) => { + const tile = { layer: 'coverage', z: 9, x: 3, y: 3 }; + await withDeferredUpstream([tile], async ({ upstream, settle }) => { + const first = fetchTile(tile); + await settle(); + upstream[0].release(); + assert.equal((await first).source, 'upstream'); + assert.equal((await fetchTile(tile)).source, 'memory'); + const now = Date.now(); + t.mock.method(Date, 'now', () => now + 25 * 60 * 60 * 1000); + const again = fetchTile(tile); + await settle(); + assert.equal(upstream.length, 2, 'neither cache answers a stale tile'); + upstream[1].release(); + assert.equal((await again).source, 'upstream'); + }); +}); + /** * A deferred upstream for in-flight tests: every fetch waits until released, * and records whether its signal was aborted. Coordinates use z9, which the diff --git a/src/ui/streetLevelControls.js b/src/ui/streetLevelControls.js index 5f879759b..4694518f4 100644 --- a/src/ui/streetLevelControls.js +++ b/src/ui/streetLevelControls.js @@ -404,6 +404,9 @@ export class StreetLevelControls { button.classList.toggle('is-active', active); button.setAttribute('aria-checked', String(active)); } + // Track open → closed here too, so the next photo gets its resize. + const wasOpen = this._viewerWasOpen; + this._viewerWasOpen = state.street.open === true; if (!viewer.open) return; if (el.imageBy) el.imageBy.textContent = viewer.captionLeft; if (el.imageWhen) el.imageWhen.textContent = viewer.captionRight; @@ -415,8 +418,7 @@ export class StreetLevelControls { // Size changes reach the viewer through the ResizeObserver; resizing on // every render ran each frame while a panorama was dragged. Opening is the // one moment the element may not have reported a size yet. - if (state.street.open && !this._viewerWasOpen) this.layer.resizeViewer?.(); - this._viewerWasOpen = state.street.open === true; + if (state.street.open && !wasOpen) this.layer.resizeViewer?.(); } _renderMeta(view) { diff --git a/src/ui/streetLevelControls.test.mjs b/src/ui/streetLevelControls.test.mjs index 1d65ee17c..c1341468d 100644 --- a/src/ui/streetLevelControls.test.mjs +++ b/src/ui/streetLevelControls.test.mjs @@ -500,4 +500,7 @@ test('the viewer is resized when it opens, not on every render (review #8)', () assert.equal(layer.calls.resize, 1, 'once on open'); for (let i = 0; i < 5; i++) layer.publish(uiState({ open: true })); assert.equal(layer.calls.resize, 1, 'not again while it stays open'); + layer.publish(uiState({ open: false })); + layer.publish(uiState({ open: true })); + assert.equal(layer.calls.resize, 2, 'the next photo resizes again'); }));