feat(url): open remote data and styles with ?data= and ?style= - #1795
Conversation
Deep links could only open saved .geolibre.json projects, so sharing a hosted GeoJSON, COG, or ZIP of GeoJSON files meant publishing a project file first. The new ?data= parameter loads that data directly, ?style= applies vector or raster symbology beside it, and a matching compact style export makes the file easy to produce from an existing layer.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe desktop app now loads remote geospatial data from URL parameters. It validates and applies styles, fits loaded layers, reports errors, and suppresses onboarding. The layer panel supports GeoLibre URL style export and import with localization and documentation. ChangesRemote data deep-link loading
GeoLibre style interchange
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserURL
participant App
participant useDataUrlLoader
participant DesktopShell
participant MapAPI
BrowserURL->>App: provide data and style parameters
App->>useDataUrlLoader: pass map API
useDataUrlLoader->>MapAPI: add remote layers and styles
useDataUrlLoader-->>DesktopShell: return load state and layer IDs
DesktopShell->>MapAPI: fit loaded layer bounds
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)
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 |
🔍 Cloudflare PR preview
|
🔍 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/DesktopShell.tsx`:
- Around line 2768-2775: Adjust the error-banner rendering in DesktopShell so
simultaneous projectUrlLoadState and dataUrlLoadState errors remain readable
instead of sharing the same absolute position. Stack the two banners vertically
or render both messages within a single positioned container, preserving each
existing error message and styling.
In `@apps/geolibre-desktop/src/components/panels/LayerPanel.tsx`:
- Around line 1693-1700: Localize the new GeoLibre style picker text: in
apps/geolibre-desktop/src/components/panels/LayerPanel.tsx lines 1693-1700, add
an English file-type translation key and pass t(...) to filters.name and
browserTypes.description; at lines 1774-1774, use the translated picker-filter
label. In apps/geolibre-desktop/src/i18n/locales/en.json line 4868, retain the
source label and update layers.importStyle across locale catalogs to include
GeoLibre URL styles.
In `@apps/geolibre-desktop/src/hooks/useDataUrlLoader.ts`:
- Around line 31-104: Guard the data URL import in the useDataUrlLoader effect
with a ref so the startup import runs at most once per session, regardless of
mapAppAPI identity changes. Mark the import as started before launching the
fetch, and adjust cleanup so completed imports are not aborted by unrelated
re-renders while still cancelling in-flight work on unmount.
In `@apps/geolibre-desktop/src/lib/data-url.ts`:
- Around line 119-134: Update the ZIP decoding flow around unzipSync to use
fflate’s asynchronous unzip API so large archives do not block the UI,
preserving the existing entry filtering and cumulative MAX_ZIP_GEOJSON_BYTES
checks. Define and throw a dedicated ZipTooLargeError for size-limit violations,
then catch it with instanceof instead of inspecting error.message; continue
wrapping other archive failures as the invalid ZIP error.
In `@docs/user-guide/embedding.md`:
- Around line 84-85: Update the `welcome` row in the onboarding documentation
table to state that the wizard is suppressed by deep links containing either
`url=` or `data=`, matching `shouldSuppressOnboarding` behavior. Leave the other
onboarding descriptions unchanged.
In `@packages/map/src/query-param-style.ts`:
- Around line 14-24: Update geoLibreStyleSourceName to strip URL fragments from
remote URLs before extracting the pathname and filename, while preserving
fragments that identify ZIP entries in the export.zip#path/file.geojson
convention. Ensure https://example.com/data.geojson#view resolves to data and
existing ZIP entry handling remains unchanged.
🪄 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: 0e5e5288-aa34-462e-9519-5af39f214856
📒 Files selected for processing (34)
apps/geolibre-desktop/src/App.tsxapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/hooks/useDataUrlLoader.tsapps/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/data-url.tsapps/geolibre-desktop/src/lib/onboarding-suppression.tsdocs/features.mddocs/index.mddocs/user-guide/embedding.mddocs/user-guide/layers.mddocs/user-guide/styling.mdpackages/map/src/index.tspackages/map/src/query-param-style.tstests/data-url.test.tstests/onboarding-suppression.test.tstests/query-param-style.test.ts
|
All three inline comments posted. Final summary below. Code reviewBugs
Quality
Security
CLAUDE.md
|
Cloud-native vector formats were the obvious gap in the data deep link: both are already loadable in the app and both stream over range requests, so a hosted GeoParquet or vector PMTiles archive can now be shared as a link and styled with ?style= like any other vector source.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user-guide/layers.md (1)
33-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument CORS requirements for both URLs.
The browser fetches the
dataandstyleURLs separately. Each remote response must allow the GeoLibre origin. State that both URLs require CORS access, especially when they use different hosts.🤖 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 `@docs/user-guide/layers.md` around lines 33 - 36, Update the GeoLibre URL style usage guidance to state that both the data and style URLs must be served with CORS access for the GeoLibre origin. Clarify that this requirement applies independently when the URLs are hosted on different domains.
🤖 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/hooks/useDataUrlLoader.ts`:
- Around line 79-83: Update the PMTiles raster/vector-style validation in the
addedLayers loop to remove the already-created control-managed layer from the
store before throwing the error, ensuring the failed deep-link import leaves no
raster layer behind. Use the existing layer-removal mechanism and preserve the
current error behavior after cleanup.
- Around line 59-82: Localize the new user-facing errors in the data-loading
flow around the PMTiles/GeoParquet layer additions by replacing the hardcoded
messages passed into DataUrlLoadState.error with react-i18next t() calls. Add
translation keys for each new message, including unsupported vector styles,
failed layer addition, missing created layers, and raster PMTiles style
incompatibility, while preserving the existing error conditions and dynamic
values.
---
Outside diff comments:
In `@docs/user-guide/layers.md`:
- Around line 33-36: Update the GeoLibre URL style usage guidance to state that
both the data and style URLs must be served with CORS access for the GeoLibre
origin. Clarify that this requirement applies independently when the URLs are
hosted on different domains.
🪄 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: 9bf2e3fc-bf7a-4467-81c1-0ed9459c598a
📒 Files selected for processing (9)
apps/geolibre-desktop/src/hooks/useDataUrlLoader.tsapps/geolibre-desktop/src/lib/data-url.tsdocs/features.mddocs/index.mddocs/user-guide/embedding.mddocs/user-guide/layers.mddocs/user-guide/styling.mdpackages/plugins/src/index.tstests/data-url.test.ts
|
All three inline comments posted. Now finalizing the summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Capture the map app API once in App.tsx: onMapReady re-fires on every basemap swap with a fresh object, which re-ran the one-shot ?data= import and duplicated its layers. - Frame only the GeoJSON layers a data link added. The COG, PMTiles, and GeoParquet loaders already move the camera, so the shell effect fit a second time; it also no longer guesses at layer-sync timing with a fixed 150ms delay, since the store bounds are available immediately. - Offset the data-URL error banner when the project-URL banner is also showing, so a link carrying both url= and data= does not hide one message behind the other. - Unzip archive members off the main thread and signal the size ceiling with a typed ZipTooLargeError instead of sniffing the message text. - Refuse a response whose advertised Content-Length exceeds the download ceiling, before buffering an arbitrary third-party body. - Read an ordinary URL hash as a hash in geoLibreStyleSourceName; only a fragment naming a .geojson/.json member identifies a ZIP entry. - Add GeoLibre URL to layers.importStyle in the 17 non-English catalogs, which still listed only Mapbox GL / SLD / QML. - Note in the embedding docs that a data= deep link also suppresses the welcome wizard.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Posted 3 inline comments covering the findings above. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/i18n/locales/fa.json (1)
4854-4854: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the established Persian label for “style.”
fa.jsonalready usesشیوهٔ نمایشfor surrounding style menu/export/import labels, includingimportStyle,exportMapboxStyle, andexportStyleError. KeepexportGeoLibreStyleconsistent by replacingسبکwithشیوهٔ نمایش.Proposed wording alignment
- "exportGeoLibreStyle": "خروجی سبک URL ژئولیبر", + "exportGeoLibreStyle": "خروجی شیوهٔ نمایش URL ژئولیبر",🤖 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/fa.json` at line 4854, Update the exportGeoLibreStyle translation in fa.json to use the established Persian label شیوهٔ نمایش instead of سبک, matching the terminology used by importStyle, exportMapboxStyle, and exportStyleError.
🤖 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 `@packages/map/src/query-param-style.ts`:
- Around line 19-21: Decode the selected ZIP fragment or source pathname before
deriving the filename stem, so encoded separators such as %2F are treated as
path delimiters before basename splitting. Update the logic around fragment/raw
selection and add a regression test covering archive.zip#folder%2Fparks.geojson,
ensuring the derived stem is parks and style matching remains successful.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/i18n/locales/fa.json`:
- Line 4854: Update the exportGeoLibreStyle translation in fa.json to use the
established Persian label شیوهٔ نمایش instead of سبک, matching the terminology
used by importStyle, exportMapboxStyle, and exportStyleError.
🪄 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: b6351bf1-3d15-4ab4-997f-420a9402393a
📒 Files selected for processing (25)
apps/geolibre-desktop/src/App.tsxapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/hooks/useDataUrlLoader.tsapps/geolibre-desktop/src/i18n/locales/ar.jsonapps/geolibre-desktop/src/i18n/locales/de.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/data-url.tsdocs/user-guide/embedding.mdpackages/map/src/query-param-style.tstests/data-url.test.tstests/query-param-style.test.ts
- Roll the PMTiles import back before reporting a raster/vector-style mismatch, and decide it over all added layers before styling any of them, so a failed deep link leaves no layer behind and applies no partial symbology. - Prefer the added layers that carry the data URL over a plain id diff, so a concurrent ?url= project load cannot make project layers look like the ones this import just created. - Enforce the download ceiling while the body streams, not only when the server advertises Content-Length, so a chunked response is cut off at the limit instead of after it has been buffered whole. - Decode a percent-encoded ZIP member fragment before taking the filename stem, which otherwise yielded "folder/parks".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/geolibre-desktop/src/hooks/useDataUrlLoader.ts (1)
90-102: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not fall back to layers that are not proven to belong to this import.
If
?url=replaces the layer array while the native loader is awaited, every project layer is insynced. IflayerPointsAtdoes not recognize the native loader source value, Line 91 selects those project layers. The raster check can then remove project layers at Line 102. Otherwise, Line 109 applies the remote style to project layers.Remove this fallback. Make
addPMTilesLayerFromUrlandaddVectorLayerFromUrlreturn the created layer IDs, or require them to persist the exactremote.urlbefore selecting or rolling back layers.#!/bin/bash set -euo pipefail # Locate native loader definitions and inspect their layer creation and return contracts. rg -n -C 8 --type ts '\b(addPMTilesLayerFromUrl|addVectorLayerFromUrl)\b' . # Inspect source-path assignment and project replacement behavior. rg -n -C 6 --type ts 'sourcePath|source:\s*\{[^}]*url|projectGeneration|setState\(\{.*layers|layers:' \ apps packages🤖 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/hooks/useDataUrlLoader.ts` around lines 90 - 102, The import flow around pointingAtData must never use all synced project layers as a fallback. Update addPMTilesLayerFromUrl and addVectorLayerFromUrl to return the IDs of layers they create, or ensure they persist the exact remote.url and select only those proven layers for styling and raster rollback; preserve failure handling when no import layers are created.
🤖 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.
Duplicate comments:
In `@apps/geolibre-desktop/src/hooks/useDataUrlLoader.ts`:
- Around line 90-102: The import flow around pointingAtData must never use all
synced project layers as a fallback. Update addPMTilesLayerFromUrl and
addVectorLayerFromUrl to return the IDs of layers they create, or ensure they
persist the exact remote.url and select only those proven layers for styling and
raster rollback; preserve failure handling when no import layers are created.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e3a1a59-7a5a-42b1-b0ce-f3652b7f0a33
📒 Files selected for processing (5)
apps/geolibre-desktop/src/hooks/useDataUrlLoader.tsapps/geolibre-desktop/src/lib/data-url.tspackages/map/src/query-param-style.tstests/data-url.test.tstests/query-param-style.test.ts
Code reviewBugs
Security
Quality
Performance / CLAUDE.md
Other things checked and found sound: the |
- Identify the layers a PMTiles/GeoParquet add created only by the data URL they record, with no fallback to the raw id diff. A concurrent project load that replaced the layer array could otherwise hand this branch the project's own layers to restyle or remove. - Read the ?style= body through the same capped reader as ?data=, so an unbounded chunked style response is cut off at the ceiling too. - Re-check the inflated ZIP total against the ceiling, instead of trusting the size each entry declares in the archive metadata. - Decode a data URL filename defensively, so a literal percent sign in the path no longer surfaces as a raw "URI malformed".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/data-url.ts`:
- Around line 178-185: Update unzipGeoJsonEntries to use fflate’s streaming
Unzip API instead of materializing all entries before validation. Track
cumulative decompressed bytes in each ondata chunk, terminate the stream and
reject with ZipTooLargeError as soon as MAX_ZIP_GEOJSON_BYTES is exceeded, while
preserving successful collection and resolution of selected GeoJSON entries.
🪄 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: e6f6a4b1-896e-4827-8542-8f826c5a0e40
📒 Files selected for processing (3)
apps/geolibre-desktop/src/hooks/useDataUrlLoader.tsapps/geolibre-desktop/src/lib/data-url.tstests/data-url.test.ts
| const layers = Object.entries(entries).map(([name, contents]) => ({ | ||
| data: parseFeatureCollection(strFromU8(contents), name), | ||
| name: name | ||
| .split("/") | ||
| .pop()! | ||
| .replace(/\.(?:geojson|json)$/i, ""), | ||
| sourcePath: `${url}#${name}`, | ||
| })); | ||
| if (!layers.length) throw new Error("The ZIP archive does not contain any GeoJSON files."); |
There was a problem hiding this comment.
The ZIP entry filter (unzipGeoJsonEntries) accepts both .geojson and .json files, but parseFeatureCollection here throws hard if any matched entry isn't a FeatureCollection. Real-world REST-export ZIPs commonly bundle a metadata.json/manifest.json alongside the actual GeoJSON payloads; one such file in the archive currently aborts the entire multi-layer import (... is not a GeoJSON FeatureCollection.) instead of importing the valid members and skipping/warning about the rest. Worth confirming this is the intended behavior, since it can turn an otherwise-successful multi-file import into a hard failure because of one unrelated .json file in the archive.
Confidence: medium.
| void Promise.all([ | ||
| fetchRemoteData(params.dataUrl, { signal: controller.signal }), | ||
| params.styleUrl ? fetchRemoteStyle(params.styleUrl, { signal: controller.signal }) : null, | ||
| ]) |
There was a problem hiding this comment.
Promise.all shares controller.signal between the data and style fetches, but nothing aborts the still-in-flight fetch when its sibling rejects first (e.g. a fast 404 on ?style= while ?data= is still streaming a large file, or vice versa). The overall load is already reported as failed via the .catch below, but the "losing" fetch keeps consuming bandwidth/CPU up to MAX_DOWNLOAD_BYTES (250 MB) for no benefit. Calling controller.abort() when either promise rejects would stop the wasted work.
Confidence: low — cosmetic/perf only, no functional impact since the result is discarded either way.
| setState({ | ||
| error: null, | ||
| fitLayerIds, | ||
| message: `Loaded ${count} layer${count === 1 ? "" : "s"} from URL`, | ||
| status: "loaded", | ||
| }); |
There was a problem hiding this comment.
state.message is populated with real user-facing copy ("Loading data from URL...", "Loaded N layer(s) from URL") but DesktopShell.tsx only ever reads dataUrlLoadState.error — .message isn't rendered anywhere. This mirrors the pre-existing ProjectUrlLoadState.message, which appears to have the same gap, but since this PR adds a brand-new consumer it's worth double-checking that a loading/success indicator was intentionally dropped and not just forgotten — otherwise a ?data= deep link that takes a while (large COG/GeoParquet, slow REST endpoint) gives the user no feedback at all until it either silently finishes or shows an error banner.
Confidence: low-medium (may be intentional/deferred, and mirrors existing pattern).
| } else if (remote.kind === "pmtiles" || remote.kind === "vector") { | ||
| // Validate the style before invoking a native loader. Those controls | ||
| // assign their own ids, so collect the newly synchronized store | ||
| // layers after the awaited add completes. | ||
| const styleResult = rawStyle === null ? null : parseMapboxStyle(rawStyle); | ||
| if (styleResult && styleResult.matchedLayerCount === 0) { | ||
| throw new Error("The remote style has no supported vector style layers."); | ||
| } | ||
| const previousIds = new Set(store.layers.map((layer) => layer.id)); | ||
| const added = | ||
| remote.kind === "pmtiles" | ||
| ? await addPMTilesLayerFromUrl(mapAppAPI, remote.url) | ||
| : await addVectorLayerFromUrl(mapAppAPI, remote.url, { | ||
| name: remote.name, | ||
| fitBounds: true, | ||
| }); | ||
| if (!added) throw new Error(`Could not add ${remote.name} to the map.`); | ||
| // These loaders assign their own ids, so the added layers have to be | ||
| // recovered from the store. Identify them by the data URL they record | ||
| // and not by an id diff alone: a concurrent `?url=` project load | ||
| // replaces the whole layer array, which would make every project | ||
| // layer look new here and hand this branch someone else's layers to | ||
| // restyle or remove. No match is treated as "could not identify", | ||
| // never as "take whatever is new". | ||
| const addedLayers = useAppStore | ||
| .getState() | ||
| .layers.filter( | ||
| (layer) => !previousIds.has(layer.id) && layerPointsAt(layer, remote.url), | ||
| ); | ||
| if (!addedLayers.length) { | ||
| throw new Error( | ||
| `The ${remote.kind === "pmtiles" ? "PMTiles" : "GeoParquet"} loader did not create a layer for ${remote.url}.`, | ||
| ); | ||
| } | ||
| // Check every added layer before styling any of them: the archive's | ||
| // tile type is only known once the control has read it, so a raster | ||
| // archive paired with a vector style has to be rolled back rather | ||
| // than left behind by a deep link that reports failure. | ||
| if (styleResult && addedLayers.some((layer) => layer.metadata.tileType === "raster")) { | ||
| for (const layer of addedLayers) store.removeLayer(layer.id); | ||
| throw new Error( | ||
| "MapLibre vector styles cannot be applied to a raster PMTiles archive.", | ||
| ); | ||
| } | ||
| if (styleResult) { | ||
| for (const layer of addedLayers) { | ||
| store.setLayerStyle(layer.id, applyMapboxStyleImport(layer.style, styleResult)); | ||
| } | ||
| } | ||
| count = addedLayers.length; |
There was a problem hiding this comment.
This is the most intricate part of the new loader — id-diffing against a concurrent ?url= project load, matching added layers by URL, rolling back a raster archive styled with a vector style, applying styles per added layer — yet it's exercised only indirectly (no test imports useDataUrlLoader itself). tests/data-url.test.ts and tests/query-param-style.test.ts cover the pure helpers (fetchRemoteData, mapboxStyleForDataLayer, parseRasterUrlStyle, buildGeoLibreQueryStyle) but not this orchestration logic. Given the concurrency edge case explicitly called out in the comments above (a project load racing this hook), a test double for the store/app API exercising this branch would catch regressions here that the current suite can't.
Confidence: medium (test-coverage gap, not a functional bug).
|
All inline comments are posted. Here's the summary. Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
Enforce the ZIP import ceiling while the entries inflate, using fflate's streaming Unzip and terminating the readers once the running total passes the limit. Checking after unzip() had materialized every entry ran after the allocation it was meant to prevent, which a ZIP that understates its declared entry sizes could exploit. The declared size is still the cheap first pass for an honestly-large archive.
| const imports = remote.layers.map((layer) => { | ||
| if (rawStyle === null) return { layer, styleResult: null }; | ||
| const styleResult = parseMapboxStyle(mapboxStyleForDataLayer(rawStyle, layer.name)); | ||
| if (styleResult.matchedLayerCount === 0) { | ||
| throw new Error( | ||
| `The remote style has no supported layers for "${layer.name}.geojson". ` + | ||
| `Set each style layer's source to the matching filename stem (for example, "${layer.name}").`, | ||
| ); | ||
| } | ||
| return { layer, styleResult }; | ||
| }); |
There was a problem hiding this comment.
mapboxStyleForDataLayer is applied here for every GeoJSON load, not only ZIP archives — remote.layers has exactly one entry for a plain single-file ?data= link too, and it goes through the same source filter.
That filter (data-url.ts mapboxStyleForDataLayer) keeps a style layer only when it has no source, or its source (stem-normalized) equals the data filename stem. A style produced by the general "Export as Mapbox GL style" (or any hand-authored/third-party Mapbox style — the spec requires source on every non-background layer) sets source to something derived from the layer name/id (buildMapboxStyle's ${idBase}-source), which will essentially never equal the raw filename stem. So pairing a single-file ?data= link with such a style throws "no supported layers" here, even though the docs and PR description describe ?style= as generically applying "Mapbox/MapLibre style JSON," and only call out source-stem binding "for a ZIP."
In practice only styles from the new Export GeoLibre URL style action (which deliberately renames its source to the stem) or a source-less style work for the single-file case. Worth either scoping this filter to the multi-layer (ZIP) case only, or calling out the source-binding requirement in the docs for single-file data=+style= too.
Confidence: medium — traced through buildMapboxStyle's sourceKey and mapboxStyleForDataLayer's matching logic, but haven't run it end-to-end in a browser.
| if (!settled && pushed && pending === 0) { | ||
| settled = true; | ||
| resolve(entries); | ||
| } | ||
| }; | ||
|
|
||
| const unzipper = new Unzip((file) => { | ||
| if (file.name.endsWith("/") || !/\.(?:geojson|json)$/i.test(file.name)) return; | ||
| const declared = file.originalSize ?? 0; | ||
| if (declared > MAX_ZIP_GEOJSON_BYTES || total + declared > MAX_ZIP_GEOJSON_BYTES) { | ||
| fail(new ZipTooLargeError()); | ||
| return; | ||
| } | ||
| const chunks: Uint8Array[] = []; | ||
| let size = 0; | ||
| pending += 1; |
There was a problem hiding this comment.
The cumulative-size guard total + declared > MAX_ZIP_GEOJSON_BYTES doesn't actually reserve capacity for entries that were already accepted but haven't started producing bytes yet: total is only incremented inside ondata as real bytes stream in (line ~207), not when an entry passes this precheck. Since fflate's Unzip callback fires for every local-file-header it discovers within a single push() call — before any of those entries' ondata has fired — several individually-under-the-cap entries whose combined declared size exceeds MAX_ZIP_GEOJSON_BYTES can all pass this check (each sees total === 0) and all get file.start()'d concurrently.
The actual cap does still get enforced once real bytes start arriving (shared total across all entries, checked in ondata), so this isn't an unbounded memory blowup, but the doc comment above (unzipGeoJsonEntries) claims the declared size is consulted "as the cheap way to reject an honestly-large archive before inflating anything" — that guarantee doesn't hold once more than one qualifying entry is present, since acceptance isn't reserved into total until decompression has already begun.
A fix would be to increment total by declared immediately upon acceptance here (and correct for the discrepancy once the real size is known), so concurrently-registered entries can't all slip past the cumulative check.
Confidence: medium — based on reading fflate's streaming Unzip API usage here; I haven't run this against a crafted multi-entry ZIP to confirm the concurrent-registration behavior in practice.
| } | ||
|
|
||
| function extension(url: string): string { | ||
| return new URL(url).pathname.split(".").pop()?.toLowerCase() ?? ""; |
There was a problem hiding this comment.
Minor: this splits the whole pathname on . rather than just the last path segment, so a URL with a dot in an earlier segment but none in the final one (e.g. /v1.2/export) yields a bogus "extension" ("2/export") instead of "". It's harmless today — the bogus value never matches the known-extension list, so it just falls through to the generic fetch+content-sniff path — but it's fragile if more extensions are added later or the fallback behavior changes.
| return new URL(url).pathname.split(".").pop()?.toLowerCase() ?? ""; | |
| function extension(url: string): string { | |
| const basename = new URL(url).pathname.split("/").pop() ?? ""; | |
| const dot = basename.lastIndexOf("."); | |
| return dot >= 0 ? basename.slice(dot + 1).toLowerCase() : ""; | |
| } |
Confidence: low — cosmetic/defensive, current behavior isn't user-visibly wrong.
Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
Summary
?data=URL parameter that opens hosted GeoJSON, a REST endpoint returning a GeoJSON FeatureCollection, a COG, or a ZIP/REST response holding several GeoJSON files (ZIP detected from headers or the PKZIP signature, so an extensionless API endpoint works). Imported layers are framed by their combined extent and the welcome wizard is suppressed for the deep link.?style=parameter applying Mapbox/MapLibre vector style JSON, or raster style JSON for a COG (mode, bands, rescale, colormap, reversed, nodata, opacity, gamma, stretch, index preset). For a ZIP, each style layer'ssourcebinds to a GeoJSON filename stem; all matches are validated before any layer is added, so a typo cannot leave a partial import..geolibre.style.jsonthat carries symbology without feature data and whose render-layer source is the original filename stem. The existing style import accepts the same file and applies it to the selected layer.Test plan
npm run test:frontend(5544 pass, 0 fail), including newtests/data-url.test.tsandtests/query-param-style.test.tspre-commit run --files <changed>clean, including thenpm buildhook and eslint?data=<geojson>&style=<style>in the web build and confirm the layer loads styled and framed?data=<zip>&style=<multi-layer style>and confirm each GeoJSON member picks up its filename-matched style?data=<cog>&style=<raster style>and confirm the raster style appliesSummary by CodeRabbit
New Features
Documentation
Bug Fixes