Share-readiness check: warn when a shared project references data recipients cannot load - #1812
Merged
Merged
Conversation
…nnot load A `.geolibre.json` is mostly references. The publish path embeds local vector data, but every tile template, COG, PMTiles endpoint, OGC service, and hosted feature service stays a URL. So a project can upload cleanly and still draw nothing for the recipient: the service wanted a token the author holds and the upload strips, the host sends no cross-origin headers so it works in the desktop app and fails in the browser viewer, or the reference never left the author's machine. The author finds out when someone tells them the map is empty, if they tell them. Add a pre-flight readiness check that runs when the Share dialog opens. `share-readiness.ts` splits it in two so the classification stays pure and testable. `collectShareSources` walks every place a layer can hide a reference (`source.url`/`data`/`tiles`/`urls`/`baseUrl`/`arcgisQueryUrl`, `metadata.originalUrl`/`tileUrl`/`localFilePath`/`localBytesUrl`, `sourcePath`) plus the basemap style and plugin manifests, and settles what needs no network: a filesystem path, a private-network or single-label host, a URL whose credential `redactUrlCredentials` will strip, a populated credential field, a query-backed layer that names no source at all. `probeShareSources` resolves the rest with one anonymous HEAD per distinct target (a ranged GET retry for hosts that refuse HEAD), capped and short-timed out. Two things the classification reuses rather than re-derives: which layers the publish path embeds comes from `isEmbeddableLocalVectorLayer`, and whether a credential survives the upload comes from the same redaction rules the upload applies. A tile template collapses to its origin before probing, because substituting a nominal 0/0/0 tile 404s on any service whose data starts deeper and would report a healthy basemap as missing. The probes deliberately go through the browser's `fetch`, not the desktop app's native HTTP bypass, so a cross-origin rejection surfaces as one instead of being masked by a request that is not subject to CORS. That is what the recipient's browser will do. The dialog lists one row per layer with its worst verdict, a plain-language reason, and a fix. It never gates the Share button: an author sharing an intranet map with intranet colleagues is doing the right thing and should not have to fight a warning. Fixes #1671
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds client-side share-readiness checks. It classifies project sources, probes eligible URLs anonymously, reports recipient accessibility issues, and displays localized guidance in the share dialog without blocking uploads. ChangesShare-readiness validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShareProjectDialog
participant checkShareReadiness
participant BrowserFetch
ShareProjectDialog->>checkShareReadiness: snapshot project sources
checkShareReadiness->>BrowserFetch: anonymous HEAD or ranged GET
BrowserFetch-->>checkShareReadiness: status or blocked/network error
checkShareReadiness-->>ShareProjectDialog: readiness report
ShareProjectDialog-->>ShareProjectDialog: render localized warnings
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx`:
- Line 2: Update the import of isEmbeddableLocalVectorLayer in
ShareProjectDialog.tsx to use its direct defining module in vector-layer-sync.ts
instead of the `@geolibre/plugins` barrel, without changing how the symbol is
used.
In `@apps/geolibre-desktop/src/lib/share-readiness.ts`:
- Around line 473-493: Update probeTarget’s request flow to create a single
timeout/deadline signal before the HEAD attempt, then reuse that signal when
composing the optional caller signal for both HEAD and ranged GET requests.
Remove the per-call AbortSignal.timeout creation inside request, while
preserving the existing retry and response-status behavior.
In `@tests/share-readiness.test.ts`:
- Around line 295-312: Update the test around probeShareSources to record each
request’s method and headers, then assert that the first attempt is HEAD and the
retry is GET with Range set to bytes=0-0. Keep the existing reachable-status
assertion so the test covers both the request contract and final outcome.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47502513-0de8-47e9-b17c-0b7adaea633f
📒 Files selected for processing (24)
apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsxapps/geolibre-desktop/src/i18n/locales/ar.jsonapps/geolibre-desktop/src/i18n/locales/de.jsonapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/i18n/locales/es.jsonapps/geolibre-desktop/src/i18n/locales/fa.jsonapps/geolibre-desktop/src/i18n/locales/fr.jsonapps/geolibre-desktop/src/i18n/locales/hi.jsonapps/geolibre-desktop/src/i18n/locales/id.jsonapps/geolibre-desktop/src/i18n/locales/it.jsonapps/geolibre-desktop/src/i18n/locales/ja.jsonapps/geolibre-desktop/src/i18n/locales/ka.jsonapps/geolibre-desktop/src/i18n/locales/ko.jsonapps/geolibre-desktop/src/i18n/locales/nl.jsonapps/geolibre-desktop/src/i18n/locales/pt.jsonapps/geolibre-desktop/src/i18n/locales/ru.jsonapps/geolibre-desktop/src/i18n/locales/th.jsonapps/geolibre-desktop/src/i18n/locales/tr.jsonapps/geolibre-desktop/src/i18n/locales/zh.jsonapps/geolibre-desktop/src/lib/share-readiness.tsdocs/features.mddocs/user-guide/projects.mdpackages/core/src/index.tstests/share-readiness.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Contributor
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
- Share one deadline across a target's HEAD attempt and its ranged-GET retry (CodeRabbit). Each attempt built its own `AbortSignal.timeout`, so a slow host that refuses HEAD could spend the budget twice and take 12s against a documented 6s per-target limit. - Assert the retry's method and `Range: bytes=0-0` header in the HEAD-refusing test (CodeRabbit). The test only checked the final status, so dropping the range header, which would have the check pull down a whole multi-gigabyte COG on every such host, would not have failed it. - Convert `share.readinessAllReachable` and `share.readinessTruncated` to i18next `_one`/`_other` plural keys across all 18 catalogs (claude-review). The manual "(s)" suffix rendered "All 1 data source(s)" and could not be fixed per language. Each locale now carries the CLDR categories it needs: the full six for Arabic, one/few/many/other for Russian, one/other for the rest, and `_other` alone for zh/ja/ko/th/id, which have a single category.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/i18n/locales/en.json`:
- Line 1221: Update the readinessTruncated_one entry first in
apps/geolibre-desktop/src/i18n/locales/en.json at lines 1221-1221 to remove the
ordinal, then align the corresponding singular entries in
apps/geolibre-desktop/src/i18n/locales/ar.json at lines 1359-1359,
apps/geolibre-desktop/src/i18n/locales/fa.json at lines 1215-1215, and
apps/geolibre-desktop/src/i18n/locales/hi.json at lines 1214-1214 with the
specified translations.
In `@apps/geolibre-desktop/src/i18n/locales/fa.json`:
- Line 1212: Update the Persian readinessAllReachable_other translation to use
count-neutral “all” wording, replacing the singular “هر {{count}} منبع داده”
phrasing while preserving the reachability meaning and count placeholder.
In `@apps/geolibre-desktop/src/i18n/locales/fr.json`:
- Line 1213: Update the French translation value for readinessNote to use
standard comparison syntax, replacing “différemment que chez vous” with
“différemment de ce qu'elles font chez vous” while preserving the rest of the
sentence.
In `@apps/geolibre-desktop/src/i18n/locales/id.json`:
- Around line 1175-1178: Update the Indonesian readinessAllReachable_other and
readinessTruncated_other translations to use count-neutral wording that reads
naturally for both singular and plural values, including replacing “Semua” with
a direct count-based phrase and using “sumber awal” instead of “sumber pertama.”
In `@apps/geolibre-desktop/src/i18n/locales/ja.json`:
- Line 1175: Update the Japanese translation value for
readinessAllReachable_other so the recipient is the subject with 受信者がアクセスできる,
while preserving the count placeholder and the existing meaning.
In `@apps/geolibre-desktop/src/i18n/locales/ru.json`:
- Line 1286: Update the Russian readinessTruncated_one translation to remove the
ordinal word “первый” while retaining the single {{count}} placeholder, so it
renders as “Проверен только {{count}} источник.” and preserves the English key’s
placeholder contract.
In `@apps/geolibre-desktop/src/lib/share-readiness.ts`:
- Line 484: Update the probe request in probeTarget() to use redirect: "error"
instead of following redirects, preventing classified or origin probes from
reaching unclassified destinations. Do not add redirect-following behavior
unless every redirect URL is reclassified before the next request.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4610fdd-0223-4d31-bdb9-49d262b8b703
📒 Files selected for processing (20)
apps/geolibre-desktop/src/i18n/locales/ar.jsonapps/geolibre-desktop/src/i18n/locales/de.jsonapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/i18n/locales/es.jsonapps/geolibre-desktop/src/i18n/locales/fa.jsonapps/geolibre-desktop/src/i18n/locales/fr.jsonapps/geolibre-desktop/src/i18n/locales/hi.jsonapps/geolibre-desktop/src/i18n/locales/id.jsonapps/geolibre-desktop/src/i18n/locales/it.jsonapps/geolibre-desktop/src/i18n/locales/ja.jsonapps/geolibre-desktop/src/i18n/locales/ka.jsonapps/geolibre-desktop/src/i18n/locales/ko.jsonapps/geolibre-desktop/src/i18n/locales/nl.jsonapps/geolibre-desktop/src/i18n/locales/pt.jsonapps/geolibre-desktop/src/i18n/locales/ru.jsonapps/geolibre-desktop/src/i18n/locales/th.jsonapps/geolibre-desktop/src/i18n/locales/tr.jsonapps/geolibre-desktop/src/i18n/locales/zh.jsonapps/geolibre-desktop/src/lib/share-readiness.tstests/share-readiness.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
…-readiness-check # Conflicts: # apps/geolibre-desktop/src/i18n/locales/ar.json # apps/geolibre-desktop/src/i18n/locales/de.json # apps/geolibre-desktop/src/i18n/locales/es.json # apps/geolibre-desktop/src/i18n/locales/fr.json # apps/geolibre-desktop/src/i18n/locales/hi.json # apps/geolibre-desktop/src/i18n/locales/id.json # apps/geolibre-desktop/src/i18n/locales/it.json # apps/geolibre-desktop/src/i18n/locales/ja.json # apps/geolibre-desktop/src/i18n/locales/ka.json # apps/geolibre-desktop/src/i18n/locales/ko.json # apps/geolibre-desktop/src/i18n/locales/nl.json # apps/geolibre-desktop/src/i18n/locales/pt.json # apps/geolibre-desktop/src/i18n/locales/ru.json # apps/geolibre-desktop/src/i18n/locales/th.json # apps/geolibre-desktop/src/i18n/locales/tr.json # apps/geolibre-desktop/src/i18n/locales/zh.json
- Drop the ordinal from the singular truncation message (CodeRabbit).
"Only the first 1 source was checked" reads as a miscount; the `_one`
form now says "Only {{count}} source was checked", and ar, fa, hi, ka,
ru, and tr drop their ordinal the same way.
- Make the Indonesian forms count-neutral (CodeRabbit). Indonesian has one
plural category, so `_other` also renders at count 1.
- Fix three wordings CodeRabbit flagged: Persian used "each" where the
message means "all", Japanese made the source rather than the recipient
the subject of access, and the French note used the non-standard
"différemment que" comparison.
- Give a caller-cancelled probe its own `aborted` reason (claude-review).
It shared `probe-budget` with "past the cap" and "no fetch available",
which is invisible today but misleading if the reason ever reaches
diagnostics.
- Recognize the RFC 6598 carrier-grade NAT range 100.64.0.0/10 as private
(claude-review), so a service reachable only there is pre-classified
rather than reported as a generic CORS failure.
- Document the one case where `embeddedLayerIds` over-trusts the publish
path (claude-review): the predicate says a layer *can* be embedded, but
data the upload cannot read back is dropped, and such a layer would ship
with neither a URL nor features after this check cleared it. Settling it
would mean a DuckDB export of every local vector layer on dialog open,
which is the cost this check exists to avoid.
Declined: switching the probe to `redirect: "error"`. Left for discussion
on the thread.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/geolibre-desktop/src/i18n/locales/es.json (3)
1227-1227: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the CORS explanation.
Line 1227 makes the browser the subject of
no envía. A browser does not send the response CORS headers. The host sends them.Use wording such as
el host no envía encabezados CORS.Suggested wording
- "readinessReasonCors": "Un navegador no puede acceder a este host: no envía encabezados entre orígenes, o no está disponible.", + "readinessReasonCors": "Un navegador no puede acceder a este host: el host no envía encabezados CORS o no está disponible.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/es.json` at line 1227, Update the Spanish translation value for readinessReasonCors so the host, rather than the browser, is identified as the sender of the CORS response headers; preserve the existing meaning and use wording equivalent to “el host no envía encabezados CORS.”
1199-1199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse natural Spanish for count-bearing messages.
These strings can produce incorrect or awkward output:
- Line 1199 uses
campo(s).- Line 3149 can render
1 teselas.- Line 3210 can render
1 de 1 imágenes.- Lines 3335-3336 can render
1 elementos.Use count-neutral wording, or matching plural keys when the English catalog defines them.
Suggested wording
- "credentialsRemoved": "No se incluyeron {{count}} campo(s) de credenciales. Los destinatarios deben proporcionar sus propias credenciales o usar una referencia intermediada.", + "credentialsRemoved": "Se omitieron campos de credenciales ({{count}} en total). Los destinatarios deben proporcionar sus propias credenciales o usar una referencia intermediada.", - "cellCount": "{{count}} teselas en la vista", + "cellCount": "Teselas en la vista: {{count}}", - "showing": "Mostrando {{shown}} de {{total}} imágenes.", + "showing": "Imágenes mostradas: {{shown}} de {{total}}.", - "showing": "Mostrando {{count}} elementos.", + "showing": "Elementos mostrados: {{count}}.", - "showingOfMatched": "Mostrando {{count}} de {{matched}} elementos.", + "showingOfMatched": "Elementos mostrados: {{count}} de {{matched}}.",Based on learnings, use count-neutral wording when a single key receives numeric interpolations unless the English catalog defines matching plural variants.
Also applies to: 3149-3149, 3210-3210, 3335-3336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/es.json` at line 1199, Update the affected Spanish locale entries, including credentialsRemoved and the messages near the referenced tile, image, and element counts, to avoid gendered or plural forms that become incorrect with numeric interpolation. Use count-neutral Spanish wording unless the English catalog provides matching plural keys, in which case align the Spanish singular and plural translations with those keys.Source: Learnings
3299-3321: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the formal Spanish voice in the STAC messages.
These new strings switch to informal imperatives such as
introduce,Elige,Selecciona,Mantén,Haz clic, andDeja. The surrounding catalog uses formal forms such asIntroduzca,Elija,Seleccione,Haga clic, andDeje.Use one form consistently.
Also applies to: 3347-3348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/es.json` around lines 3299 - 3321, The STAC strings in the Spanish locale use informal imperatives inconsistently with the catalog’s formal voice. Update the affected entries, including the additional strings around the referenced later entries, to use formal forms such as “Introduzca”, “Elija”, “Seleccione”, “Mantenga”, “Haga clic” and “Deje”, while preserving their existing meanings.apps/geolibre-desktop/src/i18n/locales/it.json (1)
1227-1227: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the CORS explanation.
The Italian text says that the browser “does not send cross-origin headers.” The service must return the required CORS response headers. Change the text to state that the host does not allow cross-origin requests or does not return the required headers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/it.json` at line 1227, Update the Italian translation value for readinessReasonCors to state that the host does not allow cross-origin requests or does not return the required CORS response headers, replacing the inaccurate claim that the browser does not send cross-origin headers.apps/geolibre-desktop/src/i18n/locales/fr.json (1)
3149-3149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect French number agreement in the changed count labels.
The single-string labels at Line [3149], Line [3210], and Lines [3335-3336] use fixed plural nouns after a count. French can display
1 tuiles,1 images, or1 éléments. Lines [5333-5334] use the same noun form in_oneand_other, so plural counts display2 réseau.Use count-neutral labels for single-string keys. Use singular and plural nouns for
diagnostics.countNetwork. Keep the noun consistent with the diagnostics description.Based on learnings, single-key locale strings with
{{count}}should use count-neutral wording when the reference key has no plural variants.Proposed wording
- "cellCount": "{{count}} tuiles dans la vue", + "cellCount": "Tuiles dans la vue : {{count}}", - "showing": "Affichage de {{shown}} images sur {{total}}.", + "showing": "Images affichées : {{shown}} sur {{total}}.", - "showing": "Affichage de {{count}} éléments.", - "showingOfMatched": "Affichage de {{count}} éléments sur {{matched}}.", + "showing": "Éléments affichés : {{count}}.", + "showingOfMatched": "Éléments affichés : {{count}} sur {{matched}}.", - "countNetwork_one": "{{count}} réseau", - "countNetwork_other": "{{count}} réseau", + "countNetwork_one": "{{count}} requête réseau", + "countNetwork_other": "{{count}} requêtes réseau",Also applies to: 3210-3210, 3335-3336, 5333-5334
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/fr.json` at line 3149, Update the French count labels at cellCount and the other affected single-string count keys to use count-neutral nouns, avoiding fixed singular or plural forms after {{count}}. In diagnostics.countNetwork, provide distinct singular and plural wording in its _one and _other variants, keeping the noun consistent with the diagnostics description.Source: Learnings
apps/geolibre-desktop/src/i18n/locales/id.json (1)
5128-5128: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the Indonesian grammar in the raster class-limit message.
Line 5128 uses
Menerapkanas a verb beforesimbologi, so the sentence is not grammatical. Use a noun phrase instead.Proposed fix
- "tooManyClasses": "Menerapkan simbologi mendukung hingga {{max}} kelas.", + "tooManyClasses": "Penerapan simbologi mendukung hingga {{max}} kelas.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/i18n/locales/id.json` at line 5128, Update the tooManyClasses translation in the Indonesian locale to use a grammatical noun phrase before “simbologi” instead of the verb “Menerapkan,” while preserving the existing {{max}} placeholder and message meaning.
♻️ Duplicate comments (1)
apps/geolibre-desktop/src/lib/share-readiness.ts (1)
482-523: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
redirect: "follow"still lets a probe reach an unclassified destination.A prior review flagged that
probeTarget()classifies only the initial target, then follows redirects withredirect: "follow", so a redirect can send the anonymous probe request to a host that was never classified as public/probeable. That comment does not show a confirmation reply (unlike the shared-deadline fix, which was confirmed addressed), and this line is unchanged in the current diff. Since the check's job is to verify what a recipient's browser would fetch, an uncontrolled redirect can silently probe a different, unvetted host.Use
redirect: "error"unless each redirect URL is re-classified before the next request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/lib/share-readiness.ts` around lines 482 - 523, Update the request options in probeTarget so redirects use “error” instead of “follow”, preventing the anonymous probe from reaching an unclassified destination; retain the existing HEAD/GET retry and shared-deadline behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/geolibre-desktop/src/i18n/locales/es.json`:
- Line 1227: Update the Spanish translation value for readinessReasonCors so the
host, rather than the browser, is identified as the sender of the CORS response
headers; preserve the existing meaning and use wording equivalent to “el host no
envía encabezados CORS.”
- Line 1199: Update the affected Spanish locale entries, including
credentialsRemoved and the messages near the referenced tile, image, and element
counts, to avoid gendered or plural forms that become incorrect with numeric
interpolation. Use count-neutral Spanish wording unless the English catalog
provides matching plural keys, in which case align the Spanish singular and
plural translations with those keys.
- Around line 3299-3321: The STAC strings in the Spanish locale use informal
imperatives inconsistently with the catalog’s formal voice. Update the affected
entries, including the additional strings around the referenced later entries,
to use formal forms such as “Introduzca”, “Elija”, “Seleccione”, “Mantenga”,
“Haga clic” and “Deje”, while preserving their existing meanings.
In `@apps/geolibre-desktop/src/i18n/locales/fr.json`:
- Line 3149: Update the French count labels at cellCount and the other affected
single-string count keys to use count-neutral nouns, avoiding fixed singular or
plural forms after {{count}}. In diagnostics.countNetwork, provide distinct
singular and plural wording in its _one and _other variants, keeping the noun
consistent with the diagnostics description.
In `@apps/geolibre-desktop/src/i18n/locales/id.json`:
- Line 5128: Update the tooManyClasses translation in the Indonesian locale to
use a grammatical noun phrase before “simbologi” instead of the verb
“Menerapkan,” while preserving the existing {{max}} placeholder and message
meaning.
In `@apps/geolibre-desktop/src/i18n/locales/it.json`:
- Line 1227: Update the Italian translation value for readinessReasonCors to
state that the host does not allow cross-origin requests or does not return the
required CORS response headers, replacing the inaccurate claim that the browser
does not send cross-origin headers.
---
Duplicate comments:
In `@apps/geolibre-desktop/src/lib/share-readiness.ts`:
- Around line 482-523: Update the request options in probeTarget so redirects
use “error” instead of “follow”, preventing the anonymous probe from reaching an
unclassified destination; retain the existing HEAD/GET retry and shared-deadline
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9b949e2-a6d2-4a56-aba3-c773b4795594
📒 Files selected for processing (20)
apps/geolibre-desktop/src/i18n/locales/ar.jsonapps/geolibre-desktop/src/i18n/locales/de.jsonapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/i18n/locales/es.jsonapps/geolibre-desktop/src/i18n/locales/fa.jsonapps/geolibre-desktop/src/i18n/locales/fr.jsonapps/geolibre-desktop/src/i18n/locales/hi.jsonapps/geolibre-desktop/src/i18n/locales/id.jsonapps/geolibre-desktop/src/i18n/locales/it.jsonapps/geolibre-desktop/src/i18n/locales/ja.jsonapps/geolibre-desktop/src/i18n/locales/ka.jsonapps/geolibre-desktop/src/i18n/locales/ko.jsonapps/geolibre-desktop/src/i18n/locales/nl.jsonapps/geolibre-desktop/src/i18n/locales/pt.jsonapps/geolibre-desktop/src/i18n/locales/ru.jsonapps/geolibre-desktop/src/i18n/locales/th.jsonapps/geolibre-desktop/src/i18n/locales/tr.jsonapps/geolibre-desktop/src/i18n/locales/zh.jsonapps/geolibre-desktop/src/lib/share-readiness.tstests/share-readiness.test.ts
💤 Files with no reviewable changes (3)
- apps/geolibre-desktop/src/i18n/locales/ar.json
- apps/geolibre-desktop/src/i18n/locales/zh.json
- apps/geolibre-desktop/src/i18n/locales/hi.json
Contributor
Code reviewI reviewed Bugs
Security
Performance
Quality
CLAUDE.md
|
- Scan for credential fields exactly as deep as the redaction pass does (claude-review). `MAX_FIELD_SCAN_DEPTH` was 6 while `MAX_REDACT_DEPTH` is 12, so a credential nested between those depths would be stripped by the upload but reported as reachable here. `MAX_REDACT_DEPTH` is now exported from core and used directly, since the point of this module is to reuse the redaction rules rather than keep a second copy of them. - Stop re-probing when the UI language changes (claude-review). The effect depended on `t` only to label the two project-level rows, so react-i18next handing out a new `t` on a language switch re-issued every network probe. The check no longer takes labels at all: it reports project-level rows with an empty `label` plus their `field`, and the dialog translates one at render time. The effect now depends on `[open, hasToken]`. Verified in the browser that the basemap row still labels correctly, and localizes (`Hintergrundkarte` under `?lang=de`).
Contributor
Code reviewBugs
Performance
Security
Quality
CLAUDE.md
|
giswqs
added a commit
that referenced
this pull request
Aug 10, 2026
…s 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.
giswqs
added a commit
that referenced
this pull request
Aug 10, 2026
…ipients cannot load (#1812) * feat(share): warn when a shared project references data recipients cannot load A `.geolibre.json` is mostly references. The publish path embeds local vector data, but every tile template, COG, PMTiles endpoint, OGC service, and hosted feature service stays a URL. So a project can upload cleanly and still draw nothing for the recipient: the service wanted a token the author holds and the upload strips, the host sends no cross-origin headers so it works in the desktop app and fails in the browser viewer, or the reference never left the author's machine. The author finds out when someone tells them the map is empty, if they tell them. Add a pre-flight readiness check that runs when the Share dialog opens. `share-readiness.ts` splits it in two so the classification stays pure and testable. `collectShareSources` walks every place a layer can hide a reference (`source.url`/`data`/`tiles`/`urls`/`baseUrl`/`arcgisQueryUrl`, `metadata.originalUrl`/`tileUrl`/`localFilePath`/`localBytesUrl`, `sourcePath`) plus the basemap style and plugin manifests, and settles what needs no network: a filesystem path, a private-network or single-label host, a URL whose credential `redactUrlCredentials` will strip, a populated credential field, a query-backed layer that names no source at all. `probeShareSources` resolves the rest with one anonymous HEAD per distinct target (a ranged GET retry for hosts that refuse HEAD), capped and short-timed out. Two things the classification reuses rather than re-derives: which layers the publish path embeds comes from `isEmbeddableLocalVectorLayer`, and whether a credential survives the upload comes from the same redaction rules the upload applies. A tile template collapses to its origin before probing, because substituting a nominal 0/0/0 tile 404s on any service whose data starts deeper and would report a healthy basemap as missing. The probes deliberately go through the browser's `fetch`, not the desktop app's native HTTP bypass, so a cross-origin rejection surfaces as one instead of being masked by a request that is not subject to CORS. That is what the recipient's browser will do. The dialog lists one row per layer with its worst verdict, a plain-language reason, and a fix. It never gates the Share button: an author sharing an intranet map with intranet colleagues is doing the right thing and should not have to fight a warning. Fixes #1671 * Address review feedback - Share one deadline across a target's HEAD attempt and its ranged-GET retry (CodeRabbit). Each attempt built its own `AbortSignal.timeout`, so a slow host that refuses HEAD could spend the budget twice and take 12s against a documented 6s per-target limit. - Assert the retry's method and `Range: bytes=0-0` header in the HEAD-refusing test (CodeRabbit). The test only checked the final status, so dropping the range header, which would have the check pull down a whole multi-gigabyte COG on every such host, would not have failed it. - Convert `share.readinessAllReachable` and `share.readinessTruncated` to i18next `_one`/`_other` plural keys across all 18 catalogs (claude-review). The manual "(s)" suffix rendered "All 1 data source(s)" and could not be fixed per language. Each locale now carries the CLDR categories it needs: the full six for Arabic, one/few/many/other for Russian, one/other for the rest, and `_other` alone for zh/ja/ko/th/id, which have a single category. * Address second review round - Drop the ordinal from the singular truncation message (CodeRabbit). "Only the first 1 source was checked" reads as a miscount; the `_one` form now says "Only {{count}} source was checked", and ar, fa, hi, ka, ru, and tr drop their ordinal the same way. - Make the Indonesian forms count-neutral (CodeRabbit). Indonesian has one plural category, so `_other` also renders at count 1. - Fix three wordings CodeRabbit flagged: Persian used "each" where the message means "all", Japanese made the source rather than the recipient the subject of access, and the French note used the non-standard "différemment que" comparison. - Give a caller-cancelled probe its own `aborted` reason (claude-review). It shared `probe-budget` with "past the cap" and "no fetch available", which is invisible today but misleading if the reason ever reaches diagnostics. - Recognize the RFC 6598 carrier-grade NAT range 100.64.0.0/10 as private (claude-review), so a service reachable only there is pre-classified rather than reported as a generic CORS failure. - Document the one case where `embeddedLayerIds` over-trusts the publish path (claude-review): the predicate says a layer *can* be embedded, but data the upload cannot read back is dropped, and such a layer would ship with neither a URL nor features after this check cleared it. Settling it would mean a DuckDB export of every local vector layer on dialog open, which is the cost this check exists to avoid. Declined: switching the probe to `redirect: "error"`. Left for discussion on the thread. * Address third review round - Scan for credential fields exactly as deep as the redaction pass does (claude-review). `MAX_FIELD_SCAN_DEPTH` was 6 while `MAX_REDACT_DEPTH` is 12, so a credential nested between those depths would be stripped by the upload but reported as reachable here. `MAX_REDACT_DEPTH` is now exported from core and used directly, since the point of this module is to reuse the redaction rules rather than keep a second copy of them. - Stop re-probing when the UI language changes (claude-review). The effect depended on `t` only to label the two project-level rows, so react-i18next handing out a new `t` on a language switch re-issued every network probe. The check no longer takes labels at all: it reports project-level rows with an empty `label` plus their `field`, and the dialog translates one at render time. The effect now depends on `[open, hasToken]`. Verified in the browser that the basemap row still labels correctly, and localizes (`Hintergrundkarte` under `?lang=de`).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1671.
A
.geolibre.jsonis mostly references. The publish path embeds local vector data, but every tile template, COG, PMTiles endpoint, OGC service, and hosted feature service stays a URL. So a project can upload cleanly and still draw nothing for the recipient, and the author finds out when someone tells them the map is empty (if they tell them).What it does
When the Share dialog opens, it classifies every data source the project references, probes the ones that need the network, and lists the ones a recipient could not load with a reason and a fix. It informs, it does not block the Share button.
Verdicts covered:
blob:, private-network or single-label host, a query-backed layer with no source at allImplementation
apps/geolibre-desktop/src/lib/share-readiness.ts, split so the classification is pure and unit-testable:collectShareSourceswalks every place a layer can hide a reference (source.url/data/tiles/urls/baseUrl/arcgisQueryUrl,metadata.originalUrl/tileUrl/localFilePath/localBytesUrl,sourcePath) plus the basemap style URL and plugin manifest URLs, de-duplicates the template an XYZ layer repeats across three fields, and settles everything that needs no network.probeShareSourcesresolves the rest: one anonymousHEADper distinct target, with a one-byte rangedGETretry for the object stores and CDNs that refuse HEAD,credentials: "omit", a 6s per-request timeout and a 16-target budget. A 5xx is reported as unchecked rather than blamed on the project.Two things it reuses rather than re-derives, so it cannot drift from the publish path:
isEmbeddableLocalVectorLayer, the same predicatebuildEmbeddedLayersuses;redactUrlCredentials/isCredentialFieldName, the same rules the upload applies.Two deliberate choices worth calling out:
0/0/0tile 404s on any service whose data starts deeper, which would report a healthy basemap as missing. The origin still answers what the check asks: is the host up, does it send cross-origin headers, does it want a credential.fetch, not the desktop app's native HTTP bypass, so a cross-origin rejection surfaces as one instead of being masked by a request that is not subject to CORS. That is what the recipient's browser will do.Testing
tests/share-readiness.test.ts: 23 new cases over classification, probing (HEAD/ranged-GET, status mapping, opaque rejection, 5xx, budget cap), and aggregation, with an injectedfetchImpl.dem.tif, an intranet WMS, a moved COG, and an inline GeoJSON layer. Four rows reported with the right verdicts; the public layer, the basemap, and the inline layer correctly stayed out of the list. A clean project shows the "All N data sources look reachable" line instead. No console errors.docs/user-guide/projects.mdand a bullet indocs/features.md.Summary by CodeRabbit
New Features
Localization
Documentation