Skip to content

Commit 829c544

Browse files
authored
Do not flag embedded deck.gl rows as unloadable in the share-readiness check (#1823)
* fix(share): do not flag embedded deck.gl rows as unloadable Two follow-ups to the share-readiness check in #1812, from review comments that landed after that PR merged. A non-GeoJSON deck.gl visualization (arc, heatmap, hexagon built from a CSV) keeps its rows in `source.data` as an array. `isPlainObject` excludes arrays, so `carriesOwnData` missed those layers, they fell through to the reference walk, and a layer whose data travels inside the project file was reported as "no source" or, when `sourcePath` still held the original CSV name, as a local file. An array `data` now counts as embedded, like an inline FeatureCollection. Only a string `data` is a URL. Separately, a rejected ranged GET no longer condemns the host. `Range` is CORS-safelisted only for a simple byte range, and an older webview may preflight it and get no matching `Access-Control-Allow-Headers` back. The HEAD that preceded the retry already proved the host answers and lets this origin read the response, so a rejection there is about the ranged request rather than the host: the probe falls back to the HEAD's verdict instead of reporting "a browser cannot fetch this host" for a host whose plain GET a renderer would fetch fine. Refs #1671. * Assert the ranged GET ran in the fallback tests Both fallback tests rejected every non-HEAD request but never checked that a GET was attempted, so they would have kept passing if the retry were dropped entirely: a bare HEAD 405 already reads as reachable and a bare 403 as credentialed. They now record each attempt through a shared helper and assert HEAD followed by GET with `Range: bytes=0-0`. Verified by deleting the retry locally, which takes the suite from 26 passing to 3 failing instead of 1.
1 parent 925d5cc commit 829c544

2 files changed

Lines changed: 92 additions & 3 deletions

File tree

apps/geolibre-desktop/src/lib/share-readiness.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,12 @@ function carriesOwnData(layer: GeoLibreLayer, embeddedLayerIds?: ReadonlySet<str
377377
if (layer.geojson) return true;
378378
const metadata = layer.metadata ?? {};
379379
if (metadata.embeddedGeoJSON) return true;
380-
return isPlainObject((layer.source ?? {}).data);
380+
const data = (layer.source ?? {}).data;
381+
// An object is an inline GeoJSON payload; an array is the row set a
382+
// non-GeoJSON deck.gl visualization (arc, heatmap, hexagon built from a CSV)
383+
// keeps in `source.data`. Both travel inside the project file. Only a *string*
384+
// `data` is a URL, and that is a reference like any other.
385+
return isPlainObject(data) || Array.isArray(data);
381386
}
382387

383388
/**
@@ -513,8 +518,22 @@ async function probeTarget(
513518
if (!RETRY_WITH_RANGED_GET.has(head.status)) return outcomeForStatus(head.status);
514519
// Plenty of object stores and CDNs refuse HEAD while serving GET happily,
515520
// so a one-byte ranged GET decides it rather than a false "needs a login".
516-
const ranged = await request("GET");
517-
return outcomeForStatus(ranged.status);
521+
try {
522+
const ranged = await request("GET");
523+
return outcomeForStatus(ranged.status);
524+
} catch (error) {
525+
const failure = classifyFetchFailure(error);
526+
if (failure.kind === "abort") return { status: "unchecked", reason: "aborted" };
527+
if (failure.kind === "timeout") return { status: "unchecked", reason: "timeout" };
528+
// The HEAD already proved the host answers and lets this origin read the
529+
// response, so a rejection here is about the ranged request rather than
530+
// the host. `Range` is CORS-safelisted only for a simple byte range, and
531+
// an older webview may preflight it and get no matching
532+
// `Access-Control-Allow-Headers` back, even though the plain GET a
533+
// renderer issues would succeed. Fall back to what HEAD said instead of
534+
// reporting a working host as unreachable.
535+
return outcomeForStatus(head.status);
536+
}
518537
} catch (error) {
519538
const failure = classifyFetchFailure(error);
520539
if (failure.kind === "abort") return { status: "unchecked", reason: "aborted" };

tests/share-readiness.test.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,23 @@ function fakeFetch(routes: Record<string, number | Error>) {
4545
return { fn, calls };
4646
}
4747

48+
/**
49+
* Answers HEAD with `headStatus` and rejects the ranged GET, recording both
50+
* attempts so a test can prove the retry actually ran.
51+
*/
52+
function rejectingRangedGet(headStatus: number) {
53+
const attempts: { method?: string; range?: string }[] = [];
54+
const fn = (async (_input: RequestInfo | URL, init?: RequestInit) => {
55+
attempts.push({
56+
method: init?.method,
57+
range: (init?.headers as Record<string, string> | undefined)?.Range,
58+
});
59+
if (init?.method === "HEAD") return new Response(null, { status: headStatus });
60+
throw new TypeError("Failed to fetch");
61+
}) as unknown as typeof fetch;
62+
return { fn, attempts };
63+
}
64+
4865
describe("isPrivateHostname", () => {
4966
it("recognizes loopback, private ranges, and reserved suffixes", () => {
5067
for (const host of [
@@ -194,6 +211,23 @@ describe("collectShareSources", () => {
194211
assert.equal(refs[0].probeUrl, "https://tiles.example.com/tileset.json");
195212
});
196213

214+
it("skips a deck.gl visualization whose rows are inlined as an array", () => {
215+
const refs = collectShareSources({
216+
layers: [
217+
layer({
218+
id: "a",
219+
name: "Arcs from CSV",
220+
type: "deckgl-viz",
221+
source: { type: "deckgl-viz", data: [{ lat: 1, lon: 2 }] },
222+
// Set when the layer is built from a local file, and not a reference
223+
// a recipient needs: the rows travel in `source.data`.
224+
sourcePath: "/home/me/flows.csv",
225+
}),
226+
],
227+
});
228+
assert.deepEqual(refs, []);
229+
});
230+
197231
it("reports a query-backed layer that names no reference at all", () => {
198232
const refs = collectShareSources({
199233
layers: [
@@ -332,6 +366,42 @@ describe("probeShareSources", () => {
332366
]);
333367
});
334368

369+
it("falls back to the HEAD verdict when only the ranged GET is rejected", async () => {
370+
const refs = collectShareSources({
371+
layers: [
372+
layer({ id: "a", name: "A", type: "cog", source: { url: "https://s3.example.com/a.tif" } }),
373+
],
374+
});
375+
// HEAD answers 405, so the host is up and readable cross-origin; the ranged
376+
// GET is rejected on its own (an older webview preflighting `Range`). That
377+
// must not turn a working host into a "blocked" verdict.
378+
const { fn, attempts } = rejectingRangedGet(405);
379+
const { refs: probed } = await probeShareSources(refs, { fetchImpl: fn });
380+
assert.equal(probed[0].status, "reachable");
381+
// Without this the test would still pass if the retry were dropped
382+
// entirely, since a bare HEAD 405 also reads as reachable.
383+
assert.deepEqual(attempts, [
384+
{ method: "HEAD", range: undefined },
385+
{ method: "GET", range: "bytes=0-0" },
386+
]);
387+
});
388+
389+
it("keeps a HEAD 403 credentialed when the ranged GET is also rejected", async () => {
390+
const refs = collectShareSources({
391+
layers: [
392+
layer({ id: "a", name: "A", type: "cog", source: { url: "https://s3.example.com/a.tif" } }),
393+
],
394+
});
395+
const { fn, attempts } = rejectingRangedGet(403);
396+
const { refs: probed } = await probeShareSources(refs, { fetchImpl: fn });
397+
assert.equal(probed[0].status, "credentialed");
398+
assert.equal(probed[0].reason, "auth-required");
399+
assert.deepEqual(attempts, [
400+
{ method: "HEAD", range: undefined },
401+
{ method: "GET", range: "bytes=0-0" },
402+
]);
403+
});
404+
335405
it("reads an opaque browser rejection as browser-blocked", async () => {
336406
const refs = collectShareSources({
337407
layers: [

0 commit comments

Comments
 (0)