fix(vector): stream large local vector files through DuckDB - #1716
Merged
Conversation
A local vector file had no size guard of any kind. Two cases froze the app with no actionable error: - A GeoJSON at or above V8's maximum string length (~537 MB) cannot be read as text at all — `readTextFile`/`File.text()` throw `RangeError: Invalid string length` before `JSON.parse` runs. The loaders' `catch` swallowed it and silently re-read the whole file through DuckDB, so the user waited through the failed attempt first. - A large zipped shapefile went to shpjs, whose `parseShp` is fully synchronous and applies the `.prj` proj4 transform per coordinate, wedging the main thread with no progress, no cancel, and no feature-count guard (that guard only runs once DuckDB has already opened the source). Add a byte-size preflight that runs on filesystem metadata (`stat`) or `File.size`, before any read: - `LARGE_VECTOR_SIZE_WARN_BYTES` (200 MB) prompts via the new `onLargeFile` option, wired alongside `onLargeDataset` at every interactive entry point (drag-drop paths and files, the desktop picker, KML import). - `MAX_TEXT_VECTOR_BYTES` diverts an oversized GeoJSON/KML/GPX/CSV straight to the DuckDB reader instead of attempting the doomed text parse. - `MAX_SHPJS_SHP_BYTES` (64 MB, measured on the uncompressed `.shp`) sends a large shapefile to DuckDB, which reads off the main thread and reports a feature count first, so the existing large-dataset guard can fire. Also document the practical ceilings and the convert-first workaround in the Adding Data guide.
Contributor
|
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:
📝 WalkthroughWalkthroughLarge vector loading now routes files at least 100 MB to DuckDB across browser and Tauri readers. Imports detect sampled non-geographic coordinates and show localized warnings. Autosave handles serialization failures without unhandled errors. Shared thresholds, documentation, and tests were updated. ChangesVector loading diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopShell
participant VectorLoader
participant DuckDB
participant CoreDiagnostics
DesktopShell->>VectorLoader: import vector layer
VectorLoader->>VectorLoader: check file size
VectorLoader->>DuckDB: load oversized vector
VectorLoader-->>DesktopShell: return imported data
DesktopShell->>CoreDiagnostics: inspect sampled coordinates
CoreDiagnostics-->>DesktopShell: return CRS diagnostic
DesktopShell-->>DesktopShell: display localized warning
Possibly related PRs
Suggested reviewers: 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 |
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: 2
🤖 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 203-222: Update the addFilePath flow’s loadDroppedVectorPaths
options to use the existing largeVectorGuards object, ensuring both
onLargeDataset and onLargeFile callbacks run for Browser Files tree additions.
Preserve the existing path-loading behavior and avoid duplicating the guard
configuration.
In `@docs/user-guide/adding-data.md`:
- Around line 28-32: Update the “Processing → Conversion” guidance in the
adding-data documentation to identify PMTiles as the format that streams map
tiles incrementally, while describing GeoParquet as a compact columnar vector
format rather than a tiled format. Preserve the existing source-format
recommendations and GeoJSON memory guidance.
🪄 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: d0f801e3-e429-4fce-b672-cfa9fb4c5b99
📒 Files selected for processing (22)
apps/geolibre-desktop/src/components/layout/DesktopShell.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/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/duckdb-vector-guard.tsapps/geolibre-desktop/src/lib/tauri-io.tsdocs/user-guide/adding-data.mdtests/duckdb-vector-guard.test.ts
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Some interactive drop/import paths still read GPX/KML/KMZ contents before the new preflight guard runs, which can still trigger the same large-text/memory failure modes this PR aims to prevent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a byte-size preflight guard to local vector imports so oversized files can be confirmed (or diverted away from text parsing / main-thread shpjs) before they’re read into memory, preventing silent freezes and double-work fallbacks in the vector loading pipeline.
Changes:
- Introduces large-vector file size thresholds + guard helpers (
confirmLargeVectorFile,exceedsTextVectorLimit,MAX_SHPJS_SHP_BYTES, etc.) alongside the existing feature-count guard. - Wires size/text-limit preflight into Tauri + browser vector loaders and routes large shapefile parses to DuckDB to avoid main-thread shpjs reprojection stalls.
- Adds user-facing documentation + localized UI string describing large-vector behavior, plus unit tests covering the new thresholds/guards.
File summaries
| File | Description |
|---|---|
| tests/duckdb-vector-guard.test.ts | Adds unit tests for the new size guard + text-limit diversion and shapefile threshold ordering. |
| docs/user-guide/adding-data.md | Documents practical large-vector ceilings and the DuckDB/convert-first guidance. |
| apps/geolibre-desktop/src/lib/tauri-io.ts | Integrates size/stat preflight, text-limit diversion, and large-shapefile-to-DuckDB routing into vector loaders. |
| apps/geolibre-desktop/src/lib/duckdb-vector-guard.ts | Defines new thresholds/types and guard helpers for large local vector files and text-size limits. |
| apps/geolibre-desktop/src/components/layout/DesktopShell.tsx | Bundles and passes large-vector guards through interactive vector-load entry points. |
| apps/geolibre-desktop/src/i18n/locales/ar.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/de.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/en.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/es.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/fr.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/hi.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/id.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/it.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/ja.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/ka.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/ko.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/nl.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/pt.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/ru.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/th.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/tr.json | Adds toolbar.item.largeVectorFileDesc translation. |
| apps/geolibre-desktop/src/i18n/locales/zh.json | Adds toolbar.item.largeVectorFileDesc translation. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Verified against a real 197 MB / 170k-polygon wetlands shapefile and an 873 MB GeoJSON in the browser. Two claims in the original comments did not survive measurement: - The text-limit diversion saves ~2s, not a full wasted read: File.text() rejects on the known size rather than after reading the file. The value is the explicit route and the log line, not the time saved. - The shpjs bypass trades total time for responsiveness rather than being a pure win: worst main-thread stall 8.9s -> 1.6s (projected .prj) and 4.7s -> 1.9s (WGS84 .prj), while overall load time rises by 3-4s. Record both numbers next to the constants so the trade-off is not rediscovered later.
Member
Author
Verified against real dataTested with the CT_Wetlands dataset (170,964 polygons; 197 MB Alongside outcome and wall-clock, each run tracks a
Both console breadcrumbs fire on the real files: Two corrections to the description aboveMeasurement contradicted two things I claimed when opening this, and the code comments have been updated to match (d539c84): The shpjs bypass is a trade-off, not a pure win. It costs 3–4s of total load time and buys a 2.4–5.5× shorter freeze. The projected-CRS row is the case that justifies it: a 8.9-second hard freeze is indistinguishable from a crash, while 13.3s of a responsive UI is merely slow. But anyone tuning The GeoJSON "double load" was overstated. I wrote that the user "waits through the failed attempt first" — measured, that wasted attempt costs ~2s, because What this does not establishThe 873 MB GeoJSON loads in ~30s on Also unchanged: the FeatureCollection still lands fully in memory, and MapLibre structured-clones it again to its own worker. The stall floor of ~1.6–1.9s across every "after" row is that residual work, and it is what off-thread ingestion would need to address. |
Contributor
🔍 GitHub Pages PR preview
|
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Replace the size-confirmation prompt with automatic routing, and lower both thresholds to the configured defaults: - DUCKDB_VECTOR_ROUTE_BYTES (100 MB): at or above this, a local vector file skips the in-memory JavaScript readers (JSON.parse for GeoJSON, shpjs for shapefiles, the KML/GPX/CSV text readers) and streams through DuckDB instead — off the main thread, no prompt, just a console note. For a zipped shapefile the threshold is measured on the uncompressed .shp, since shapefiles compress heavily. - DUCKDB_VECTOR_FEATURE_WARN_COUNT: 500,000 -> 100,000. This stays a prompt, because it is about materializing features in memory, which file size does not predict for compressed formats. One threshold now covers every format, replacing the separate 64 MB shapefile rule. MAX_TEXT_VECTOR_BYTES is gone: routing at 100 MB is far below V8's 536,870,888-byte string cap, so the RangeError it guarded is unreachable. Removes onLargeFile, confirmLargeVectorFile, LargeVectorFile, and the largeVectorFileDesc string from all 17 locales.
Contributor
|
Now finalizing the review summary. Code reviewBugs
Performance
Quality
Security
CLAUDE.md adherence
I verified |
…d CRS, and stop autosave crashing on large projects Three fixes to the large-vector handling, all found while testing a real 170k-polygon wetlands dataset. 1. Unify the Add Vector Layer threshold. The panel is backed by maplibre-gl-vector, whose autoThreshold defaulted to 25 MB / 50k features, while the drag-and-drop loaders switched to DuckDB at 100 MB / 100k. The same file therefore behaved differently depending on how it was added, and no single limit could be documented. Both thresholds now live in @geolibre/core and are passed to the control, so the two entry points agree. 2. Warn when a layer's coordinates cannot be WGS84. detectNonGeographicCoordinates samples a collection and reports coordinates outside +/-180 / +/-90 — the case where a file declares CRS84 or GCS_WGS_1984 but holds projected easting/northing. GeoLibre honours the declared CRS, so such a layer loads cleanly, lists in the Layers panel, and renders nowhere with no error at all. It now says so, in its own toast: the drop handler sets a success message after the layers are added, which would clobber a shared one, and both messages are true at once. 3. Stop autosave crashing. serializeProject runs synchronously outside the promise chain in useProjectHistory, so a project embedding a large vector layer threw an unhandled `RangeError: Invalid string length` (V8's 536,870,888-byte string cap) on every autosave tick. Autosave is best-effort and must degrade to "no crash recovery", not to a crash. Verified against the mislabelled CT_Wetlands data: the warning fires with the real out-of-range values and stays quiet on the reprojected copy.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/useProjectHistory.ts`:
- Around line 87-92: Separate buildProjectSnapshot(mapControllerRef) from
serializeProject in the autosave flow: catch snapshot-construction errors and
report them as autosave errors, while keeping serialization failures non-fatal
with the existing “project is too large to serialize” warning and early return.
Ensure both failure paths preserve the surrounding autosave behavior.
In `@apps/geolibre-desktop/src/i18n/locales/en.json`:
- Line 758: Make the aggregated layer warning count-neutral in
apps/geolibre-desktop/src/i18n/locales/en.json:758-758, ar.json:827-827,
de.json:747-747, es.json:747-747, fr.json:747-747, hi.json:747-747,
it.json:747-747, and ka.json:747-747 by replacing the identified singular
pronouns, agreement, and layer-visibility wording with language-appropriate
phrasing that works for one or multiple names; update only these translation
strings and preserve the warning’s meaning.
In `@apps/geolibre-desktop/src/i18n/locales/nl.json`:
- Line 747: Update the nonGeographicCoordinates translations to use
plural-neutral wording for the aggregated {{names}} value: in
apps/geolibre-desktop/src/i18n/locales/nl.json lines 747-747, use plural verbs
and “layers”; in apps/geolibre-desktop/src/i18n/locales/pt.json lines 747-747,
use plural verbs and a plural subject; and in
apps/geolibre-desktop/src/i18n/locales/ru.json lines 787-787, use plural verbs,
pronouns, and “layers.”
In `@packages/core/src/types.ts`:
- Around line 1902-1905: Update the GeoJSON feature traversal around the
geometry coordinate handling to detect GeometryCollection objects and
recursively visit each child in geometry.geometries, preserving the existing
sampled limit across all nested geometries. Continue visiting direct
geometry.coordinates as before, and add coverage for projected coordinates
nested within a GeometryCollection.
In `@tests/duckdb-vector-guard.test.ts`:
- Around line 148-157: Update the “stops at the sample limit instead of walking
the whole collection” test to place an out-of-range coordinate feature at index
25, then keep the assertion that detectNonGeographicCoordinates called with
sampleLimit 25 returns null, making the limit behavior observable.
🪄 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: 93d52ca6-9578-4943-a53f-cace3441c2b6
📒 Files selected for processing (23)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/hooks/useProjectHistory.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/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/duckdb-vector-guard.tspackages/core/src/types.tspackages/plugins/src/plugins/maplibre-vector.tstests/duckdb-vector-guard.test.ts
- Keep delimited text on the JS parser at any size. loadDuckDbVectorFile has no lon/lat column detection (that lives only in the GeoParquet conversion path), so routing a plain lon/lat CSV over 100 MB to DuckDB failed with "DuckDB did not find a geometry column in this file" for files that loaded before. Reported on both loadTauriVectorFile and loadBrowserVectorFile. - Do not abort a drop batch on an oversized GPX/KML. Those branches read the whole document as text to extract ground overlays and models before the guarded loader runs; that read now decodes from bytes past the string cap, and a failure yields no overlays instead of throwing out the whole batch. - Only log the DuckDB route for extensions the flag actually gates. zip/kmz unpack first and decide from the uncompressed .shp, so the container's own size made the message misleading near the threshold. - Visit GeometryCollection members in detectNonGeographicCoordinates. Their coordinates sit under geometries[], so such features were skipped entirely and an all-GeometryCollection file passed the check silently. - Clear the CRS warning when a new drop starts, so it cannot linger over an unrelated drop the way dropMessage/dropError cannot. - Correct the docs: PMTiles is the tiled format; GeoParquet is columnar and compact but not tiled.
Contributor
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/adding-data.md (1)
22-23: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not describe DuckDB routing as streaming.
loadDuckDbVectorFilereturns a completeFeatureCollection, so conversion still materializes all features before the caller receives them. “Streams the file” can imply incremental or bounded-memory loading. Describe this as routing through DuckDB in a worker.Proposed correction
- **100 MB or larger**, GeoLibre streams the file through DuckDB instead — - off the main thread, so the interface keeps responding. For a zipped + **100 MB or larger**, GeoLibre routes the file through DuckDB instead of + the in-memory reader. DuckDB runs off the main thread, so the interface + keeps responding. For a zipped🤖 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/adding-data.md` around lines 22 - 23, Update the documentation near the 100 MB threshold to describe routing the file through DuckDB in a worker, not streaming it. Clarify that processing occurs off the main thread while avoiding any implication of incremental or bounded-memory feature loading.
♻️ Duplicate comments (1)
packages/core/src/types.ts (1)
1904-1915: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTraverse nested
GeometryCollectionmembers.Line 1911 visits only direct members with
coordinates. A valid nestedGeometryCollectionhasgeometriesinstead, so projected coordinates below that level do not produce a CRS warning.Use a recursive geometry visitor. Add a nested-collection test.
#!/bin/bash set -euo pipefail tmp_dir="$(mktemp -d)" trap 'rm -rf "$tmp_dir"' EXIT npm pack --pack-destination "$tmp_dir" `@types/geojson`@7946.0.16 >/dev/null archive="$(find "$tmp_dir" -name '*.tgz' -print -quit)" types_file="$(tar -tzf "$archive" | rg '(^|/)index\.d\.ts$' | head -n1)" tar -xOf "$archive" "$types_file" | rg -n -A4 -B2 'GeometryCollection|geometries' rg -n -C3 'geometry\.geometries|visitGeometry|detectNonGeographicCoordinates' \ packages/core/src/types.ts tests/duckdb-vector-guard.test.ts🤖 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 `@packages/core/src/types.ts` around lines 1904 - 1915, Update the geometry traversal in the CRS-detection visitor around the existing geometry handling to recurse through each member’s own geometries collection, not just direct coordinates. Preserve the sampled limit while descending nested GeometryCollection structures, and add a test covering projected coordinates inside a nested collection so the CRS warning is produced.
🤖 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 1571-1573: Update the browser handleDrop flow to call
setCrsWarning(null) alongside the existing browser status reset, matching the
native drop handler behavior and clearing stale CRS warnings when a new browser
drop begins.
In `@apps/geolibre-desktop/src/lib/tauri-io.ts`:
- Around line 2842-2844: Update the dropped-file GPX branch to use
loadBrowserVectorFile for files at or above the existing 100 MB threshold,
retaining parseGpxTextLayers only for smaller files. Apply the same size-based
routing in the Tauri GPX batch path near its GPX handling logic, while
preserving existing layer-loading behavior.
---
Outside diff comments:
In `@docs/user-guide/adding-data.md`:
- Around line 22-23: Update the documentation near the 100 MB threshold to
describe routing the file through DuckDB in a worker, not streaming it. Clarify
that processing occurs off the main thread while avoiding any implication of
incremental or bounded-memory feature loading.
---
Duplicate comments:
In `@packages/core/src/types.ts`:
- Around line 1904-1915: Update the geometry traversal in the CRS-detection
visitor around the existing geometry handling to recurse through each member’s
own geometries collection, not just direct coordinates. Preserve the sampled
limit while descending nested GeometryCollection structures, and add a test
covering projected coordinates inside a nested collection so the CRS warning is
produced.
🪄 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: a59a03ea-4614-4aea-a913-3dde5e69f6b9
📒 Files selected for processing (5)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/lib/tauri-io.tsdocs/user-guide/adding-data.mdpackages/core/src/types.tstests/duckdb-vector-guard.test.ts
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Separate snapshot construction from serialization in the autosave path. The single try block also covered buildProjectSnapshot, so a construction failure was misreported as "the project is too large". - Make the mislabelled-CRS warning count-neutral in all 17 locales. The importer aggregates layer names into one message, so wording that agreed with a singular subject was wrong for a multi-file drop. - Route an oversized GPX through the guarded loader instead of decoding the whole document into one string. The DuckDB reader returns a single merged layer rather than the waypoint/track/route split, which is the accepted trade at that size. - Clear the CRS warning when a browser drop starts. It was previously cleared only on the 4s status timeout, which both missed the browser handler and wrongly auto-dismissed a warning that has its own Close button. - Make the sampling-limit test observable by placing an out-of-range feature past the limit; the all-valid collection passed even if sampleLimit were ignored.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Gate the maxAbsX/maxAbsY update on Number.isFinite, matching the finiteness check already used for the offending determination. A single Infinity would otherwise be reported as the offending magnitude and hide the real value. - Clear the CRS warning in the KML import handler too. Its catch sets dropError without reaching addImportedVectorLayers, so a previous file's banner could sit beside the new error.
Contributor
|
Both inline comments posted. Now the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Picks up opengeos/maplibre-gl-vector#59, which fixes two bugs the Add Vector Layer panel hit on a large, custom-CRS GeoPackage: - A layer whose GeoJSON document exceeded the maximum JavaScript string length failed to ingest with "Invalid string length". It is now encoded feature by feature and ingested in batches. - A layer whose gpkg_spatial_ref_sys row is not an EPSG row (organization "CUSTOM", carrying a WKT definition) was never reprojected, so it rendered off the map with "Invalid LngLat latitude value" and no explanation. Per the bump checklist in CLAUDE.md, MAX_VECTOR_BYTES in packages/plugins/src/plugins/remote-file-formats.ts was re-checked against MAX_REMOTE_FILE_BYTES in the package's src/lib/utils/remote.ts: both are still 2 ** 31 - 1, so the mirror needs no change. Verified in the built web app: a GeoPackage with a CUSTOM-organization Albers SRS now reprojects and renders over Connecticut with zero diagnostics.
Contributor
Code reviewBugs
Quality
Nothing found in Security or Performance beyond what's noted above — the size/feature-count guards, |
- Stop routing large GPX through DuckDB. `loadDuckDbVector` passes no `layer` argument, so `ST_Read` reads only a GPX's first OGR layer (usually `waypoints`) and silently discarded its tracks and routes — data loss, not the "single merged layer" the comment claimed. GPX now always uses `parseGpxTextLayers`, matching the decision already made for delimited text. - Drop `readVectorFileText`. `TextDecoder.decode()` over the whole buffer builds one JS string exactly as `File.text()` does, so it hit the same RangeError one line later; the comment claimed an avoidance that never existed. `readVectorFileTextOrEmpty` now calls `file.text()` directly and keeps the catch, which is what actually made an oversized KML survivable. - Report a non-RangeError serialization failure as an error rather than filing it under "the project is too large", which would have hidden that class of bug.
giswqs
changed the title
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
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.
Problem
A user reported that a zipped shapefile of 18,000 polygons (148 MB) and the same data as GeoJSON (539 MB) both freeze the app, while smaller files in the same formats load fine. There was no size guard of any kind on local vector loads.
readLocalFileTextmaterializes the whole file as one JS string, thenJSON.parseruns synchronously. V8 caps a single string at 536,870,888 bytes, so a large file throwsRangeError: Invalid string lengthbefore parsing starts.parseShpis fully synchronous and applies the.prjproj4 transform per coordinate. Measured on a real 197 MB / 170k-polygon shapefile: an 8.9 second main-thread freeze with a projected.prj.DUCKDB_VECTOR_FEATURE_WARN_COUNTis a feature count checked after DuckDB has opened the source; the shpjs andJSON.parsepaths bypassed it entirely.What this does
One threshold, no prompt.
DUCKDB_VECTOR_ROUTE_BYTES(100 MB): at or above it a local vector file skips the in-memory JavaScript readers and streams through DuckDB instead — off the main thread, decided from filesystem metadata before a byte is read. For a zipped shapefile it is measured on the uncompressed.shp, since shapefiles compress heavily.DUCKDB_VECTOR_FEATURE_WARN_COUNTdrops to 100,000 and remains a prompt, because it is about materializing features in memory, which file size does not predict for compressed formats. Both live in@geolibre/coreso the Add Vector Layer panel (which configuresmaplibre-gl-vector'sautoThreshold) switches at the same numbers as drag-and-drop.Two formats are deliberately excluded from routing: delimited text, because
loadDuckDbVectorFilecannot build points from lon/lat columns, and GPX, becauseST_Readwithout alayerargument returns only the first OGR layer and would discard tracks and routes.Warn on a mislabelled CRS.
detectNonGeographicCoordinatessamples a collection and reports coordinates outside ±180 / ±90 — a file declaringCRS84while holding projected easting/northing. GeoLibre honours the declared CRS, so such a layer previously loaded cleanly, appeared in the Layers panel, and rendered nowhere with no error at all.Stop autosave crashing.
serializeProjectruns synchronously outside the promise chain inuseProjectHistory, so a project embedding a large vector layer threw an unhandledRangeErroron every autosave tick.Bump
maplibre-gl-vectorto 0.10.8, picking up opengeos/maplibre-gl-vector#59: large GeoPackage layers no longer fail withInvalid string length, and layers whose spatial-reference row is not an EPSG row now reproject from their stored WKT.Verification
Measured in the built web app with a real 170k-polygon wetlands dataset, tracking main-thread stalls via a
requestAnimationFrameticker:.shp, projected.prj.shp, WGS84.prjRouting trades total time for responsiveness rather than being a pure win: it costs 3-4 seconds to buy a 2.4-5.5x shorter freeze. A nine-second freeze reads as a crash; 13 seconds of a responsive UI reads as slow.
npm run test:frontend- 5236 passnpm run typecheck,npm run test:e2e(29 pass), and pre-commit cleanNot addressed
The FeatureCollection still lands fully in memory once loaded, and MapLibre structured-clones it again to its own worker. The ~1.6-1.9s residual stall in every "after" row is that work. A 1.4 GB / 430k-feature GeoPackage now completes ingest and reprojection but still exhausts the renderer during tile generation.