From e3bc431f9a641952b2ea14772d38d50566881647 Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Thu, 15 Oct 2026 16:08:18 +0100 Subject: [PATCH] =?UTF-8?q?fix(street-level):=20review=20follow-ups=20?= =?UTF-8?q?=E2=80=94=20shares=20judged=20against=20the=20last=20draw;=20re?= =?UTF-8?q?fused=20pictures=20stay=20refused?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the maintainer review of #1134 (approve with follow-ups): - lineCoverage: a share is judged against the cap the tile was last drawn with, not against later shares that redrew nothing, so small increases add up (a retained tile stayed at 100 points after its share grew to 116). A rebuild's measuring pass no longer counts as a draw. - photoViewer: a picture refused for its size (bytes, streamed bytes or decoded side) is marked `tooLarge`, and the viewer never loads that file again by any path; the flat fallback had retried an oversized SD file through an , which has no size limit. - Regressions for both: cumulative share growth below the threshold, and a refused SD panorama that is not fetched again as a flat photo. Co-Authored-By: Claude Opus 5.5 --- src/layers/streetLevel/lineCoverage.js | 16 +++++--- src/layers/streetLevel/lineCoverage.test.mjs | 40 +++++++++++++++++++ src/layers/streetLevel/photoViewer.js | 34 ++++++++++++---- src/layers/streetLevel/photoViewer.test.mjs | 19 +++++++-- .../providers/panoramax/viewer.test.mjs | 36 +++++++++++++++++ 5 files changed, 129 insertions(+), 16 deletions(-) diff --git a/src/layers/streetLevel/lineCoverage.js b/src/layers/streetLevel/lineCoverage.js index 78c9b36da..ab9b996db 100644 --- a/src/layers/streetLevel/lineCoverage.js +++ b/src/layers/streetLevel/lineCoverage.js @@ -152,9 +152,10 @@ export function createLineCoverage({ /** * A tile's sequences, filtered then capped: capping first could leave a - * dense tile of newer flat captures with no 360° lines at all. + * dense tile of newer flat captures with no 360° lines at all. `measure` + * only records what the lines need, for a draw that follows. */ - function drawnSequences(entry) { + function drawnSequences(entry, { measure = false } = {}) { const drawn = []; const current = filter(); const selectedId = state.sequence.selectedId ?? null; @@ -180,7 +181,12 @@ export function createLineCoverage({ } // What every passing line would take: the budget is shared by it. entry.pointDemand = demand; - entry.pointsDrawn = points; + if (!measure) { + entry.pointsDrawn = points; + // The cap this draw used: a share is judged against it, not against + // later shares that redrew nothing (small steps add up). + entry.drawnCap = cap; + } return drawn; } @@ -204,7 +210,7 @@ export function createLineCoverage({ const share = left / (byDemand.length - i); const cap = Math.floor(Math.min(entry.pointDemand || 0, share)); left -= cap; - const before = entry.pointCap ?? perTilePointCap; + const before = entry.drawnCap ?? perTilePointCap; entry.pointCap = cap; // Only a change that alters what is drawn, by enough to be worth a // rebuild: more room for a tile that had lines left out, or well @@ -899,7 +905,7 @@ export function createLineCoverage({ // What each tile's lines need under the new filter, then the shares, // then one redraw each. for (const entry of state.coverage.tiles.values()) - if (entry.kind === 'sequence') drawnSequences(entry); + if (entry.kind === 'sequence') drawnSequences(entry, { measure: true }); shareCaps(); for (const entry of state.coverage.tiles.values()) redrawEntry(entry); requestRender(); diff --git a/src/layers/streetLevel/lineCoverage.test.mjs b/src/layers/streetLevel/lineCoverage.test.mjs index db89e8143..b0b5f7e94 100644 --- a/src/layers/streetLevel/lineCoverage.test.mjs +++ b/src/layers/streetLevel/lineCoverage.test.mjs @@ -433,3 +433,43 @@ test('a selected line outside the cap does not make its tile redraw on every set for (let i = 0; i < 3; i++) coverage.recolorSequence('short', true); assert.equal(after.added, 0); }); + +test('small share increases add up: a retained tile redraws once its share has grown past the threshold since its last draw', async (t) => { + const tuning = { + ...TUNING, + maxTiles: 1, + maxSequences: 900, + maxPoints: 100, + keepOrder: true, + }; + const { state, coverage } = setup(t, { + tuning, + decoded: (centre, tile) => ({ + sequences: Array.from({ length: 30 }, (_, i) => + pointsLine(`l${i}`, 4, tile, centre), + ), + overview: [], + }), + }); + coverage.refresh(); + assert.ok( + await until( + () => state.coverage.tiles.size === 1 && !state.coverage.pending.size, + ), + ); + const [tile] = state.coverage.tiles.values(); + assert.equal(tile.pointsDrawn, 100); + const settle = async () => { + coverage.refresh(); + await until(() => !state.coverage.pending.size); + await new Promise((resolve) => setTimeout(resolve, 10)); + }; + // +8%: under the threshold, nothing is redrawn. + tuning.maxPoints = 108; + await settle(); + assert.equal(tile.pointsDrawn, 100); + // +8% again: +16% since the tile was last drawn, so it is redrawn. + tuning.maxPoints = 116; + await settle(); + assert.equal(tile.pointsDrawn, 116, 'the grown share is drawn'); +}); diff --git a/src/layers/streetLevel/photoViewer.js b/src/layers/streetLevel/photoViewer.js index a7e86cb42..3de2c1af7 100644 --- a/src/layers/streetLevel/photoViewer.js +++ b/src/layers/streetLevel/photoViewer.js @@ -78,7 +78,7 @@ async function loadBitmap(url, timeoutMs, resize, failure, signal) { // Picture hosts are not ours: refuse a file or an image past any real // photo's size before it is decoded. const length = Number(response.headers?.get?.('content-length')); - if (length > MAX_IMAGE_BYTES) throw new Error(failure); + if (length > MAX_IMAGE_BYTES) throw oversized(failure); const blob = await boundedBlob(response, failure); const bitmap = await (resize ? createImageBitmap(blob, { @@ -90,10 +90,12 @@ async function loadBitmap(url, timeoutMs, resize, failure, signal) { // (A resized decode reports its new size: this guards unresized ones.) if (bitmap.width > MAX_IMAGE_SIDE || bitmap.height > MAX_IMAGE_SIDE) { bitmap.close?.(); - throw new Error(failure); + throw oversized(failure); } return bitmap; - } catch { + } catch (error) { + // A size refusal stays one: the viewer must not fetch it another way. + if (error?.tooLarge) throw error; throw new Error(failure); } finally { clearTimeout(timer); @@ -101,6 +103,14 @@ async function loadBitmap(url, timeoutMs, resize, failure, signal) { } } +/** + * A picture refused for its size (`tooLarge`): the viewer will not load + * that file another way, an having no size limit at all. + */ +export function oversized(failure) { + return Object.assign(new Error(failure), { tooLarge: true }); +} + /** * The body as a Blob, read with a running count so a host that sends no * length (or a false one) cannot fill memory before the cap is checked. @@ -109,7 +119,7 @@ async function boundedBlob(response, failure) { const reader = response.body?.getReader?.(); if (!reader) { const blob = await response.blob(); - if (blob.size > MAX_IMAGE_BYTES) throw new Error(failure); + if (blob.size > MAX_IMAGE_BYTES) throw oversized(failure); return blob; } const chunks = []; @@ -120,7 +130,7 @@ async function boundedBlob(response, failure) { total += value.byteLength; if (total > MAX_IMAGE_BYTES) { reader.cancel().catch(() => {}); - throw new Error(failure); + throw oversized(failure); } chunks.push(value); } @@ -226,8 +236,17 @@ export function createPhotoViewer({ /** ‹ › presses run one after another, each from where the last one went. */ let steps = Promise.resolve(); - const loadAs = (url, options) => - load(url, { failure: LOAD_FAILED, ...options }); + /** Files refused for their size: never loaded again, by any path. */ + const refused = new Set(); + async function loadAs(url, options) { + if (refused.has(url)) throw oversized(LOAD_FAILED); + try { + return await load(url, { failure: LOAD_FAILED, ...options }); + } catch (error) { + if (error?.tooLarge) refused.add(url); + throw error; + } + } function requestRender() { render?.governorRequestRender?.(`${providerId}-viewer`); @@ -796,6 +815,7 @@ export function createPhotoViewer({ unmount() { openSeq++; dropSharpPanorama(); + refused.clear(); openingId = null; keepViewFor = null; flatSharpUrl = null; diff --git a/src/layers/streetLevel/photoViewer.test.mjs b/src/layers/streetLevel/photoViewer.test.mjs index 05391a3cc..758a1e25b 100644 --- a/src/layers/streetLevel/photoViewer.test.mjs +++ b/src/layers/streetLevel/photoViewer.test.mjs @@ -296,12 +296,17 @@ test('a picture file or bitmap past any real photo’s size is refused', async ( headers: new Map([['content-length', String(length ?? '')]]), blob: async () => ({ size: size ?? 10, bitmap: bitmap(width, height) }), })); + const refusedForSize = (error) => { + assert.match(error.message, /could not be loaded/); + assert.equal(error.tooLarge, true, 'marked as a size refusal'); + return true; + }; respond({ length: MAX_IMAGE_BYTES + 1 }); - await assert.rejects(loadImage('https://x/a.jpg'), /could not be loaded/); + await assert.rejects(loadImage('https://x/a.jpg'), refusedForSize); respond({ size: MAX_IMAGE_BYTES + 1 }); - await assert.rejects(loadImage('https://x/a.jpg'), /could not be loaded/); + await assert.rejects(loadImage('https://x/a.jpg'), refusedForSize); respond({ width: MAX_IMAGE_SIDE + 1 }); - await assert.rejects(loadImage('https://x/a.jpg'), /could not be loaded/); + await assert.rejects(loadImage('https://x/a.jpg'), refusedForSize); assert.equal(closed, 1, 'the oversize bitmap is released'); // No length given, a body that keeps coming: cut off while it streams. let pulled = 0; @@ -315,8 +320,14 @@ test('a picture file or bitmap past any real photo’s size is refused', async ( }, }), }); - await assert.rejects(loadImage('https://x/a.jpg'), /could not be loaded/); + await assert.rejects(loadImage('https://x/a.jpg'), refusedForSize); assert.ok(pulled * 8 * 1024 * 1024 <= MAX_IMAGE_BYTES + 16 * 1024 * 1024); + // Other failures are not size refusals. + globalThis.fetch = async () => ({ ok: false, headers: new Map() }); + await assert.rejects( + loadImage('https://x/a.jpg'), + (error) => !error.tooLarge, + ); respond({}); assert.equal((await loadImage('https://x/a.jpg')).width, 100); }); diff --git a/src/layers/streetLevel/providers/panoramax/viewer.test.mjs b/src/layers/streetLevel/providers/panoramax/viewer.test.mjs index 78c39f316..614a50e13 100644 --- a/src/layers/streetLevel/providers/panoramax/viewer.test.mjs +++ b/src/layers/streetLevel/providers/panoramax/viewer.test.mjs @@ -295,3 +295,39 @@ test('a photo with no author to name is captioned with the instance holding it', const named = { ...photo, creator: 'Robot8A' }; assert.equal(PANORAMAX_SOURCES.describe(named).title, null); }); + +test('an SD panorama refused for its size is not fetched again as a flat photo', async () => { + const loads = []; + const viewer = createPanoramaxViewer({ + getPhoto: async () => PHOTOS.pano, + step: async () => null, + onStep() {}, + load: async (url, options = {}) => { + loads.push({ url, crossOrigin: options.crossOrigin }); + // The picture host's SD file is past the viewer's size limits. + if (url === PHOTOS.pano.urls.sd) + throw Object.assign(new Error('too big'), { tooLarge: true }); + return { src: url }; + }, + createRenderer: () => ({ + setImage() {}, + setView() {}, + getView: () => ({ yaw: 0, pitch: 0, fov: 80 }), + resize() {}, + clear() {}, + destroy() {}, + }), + }); + const host = fakeHost(); + await viewer.mount(host); + await viewer.open('pano'); + const sdLoads = loads.filter((l) => l.url === PHOTOS.pano.urls.sd); + assert.equal(sdLoads.length, 1, 'tried once, as a bitmap, and refused'); + assert.equal(sdLoads[0].crossOrigin, true); + const { picture } = parts(host); + assert.equal(picture.hidden, false, 'the flat fallback shows'); + assert.notEqual(picture.src, PHOTOS.pano.urls.sd); + // Opened again later: the refused file is still not asked for. + await viewer.open('pano'); + assert.equal(loads.filter((l) => l.url === PHOTOS.pano.urls.sd).length, 1); +});