feat: split dropped GPX files into named layers - #109
Conversation
Dragging or dropping a GPX file now yields separate Waypoints, Tracks, and Routes layers with descriptive names instead of a single merged layer, so each geometry type can be styled and toggled independently.
|
Netlify preview: https://pr-109--opengeos.netlify.app |
There was a problem hiding this comment.
Pull request overview
This PR improves GPX drag-and-drop imports in the GeoLibre Desktop app by splitting a dropped GPX into separate vector layers for Waypoints, Tracks, and Routes, while keeping the “Add Data” vector-file flow loading GPX as a single merged layer.
Changes:
- Added GPX parsing helpers that can either merge GPX content into one
FeatureCollectionor return multiple namedLoadedVectorLayers (Waypoints/Tracks/Routes) for drag-and-drop imports. - Updated dropped-file loading (browser + Tauri paths) to split GPX into multiple layers with descriptive names derived from the file name.
- Updated the desktop shell import path so it honors an optional provided
layer.namewhen adding layers to the map.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/geolibre-desktop/src/lib/tauri-io.ts | Adds LoadedVectorLayer support and GPX splitting for drag-and-drop while preserving merged GPX loading for “open vector file” flows. |
| apps/geolibre-desktop/src/components/layout/DesktopShell.tsx | Uses layer.name (when present) instead of always deriving the layer name from the source path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const layer of importedLayers) { | ||
| lastLayerId = addGeoJsonLayer( | ||
| layerNameFromPath(layer.path), | ||
| layer.name ?? layerNameFromPath(layer.path), |
There was a problem hiding this comment.
UX regression (medium confidence): addImportedVectorLayers only calls fitLayer on the last added layer. Before this PR a GPX always produced one layer, so the fit was always correct. Now it can produce up to three — the last one is always Routes (after the [Waypoints, Tracks, Routes] ordering in parseGpxTextLayers). For a file with only waypoints and tracks the view zooms to the tracks layer; for a file with all three it zooms to routes — the user may not see their waypoints at all if they're in a different region.
A simple fix is to compute a union bounding box across all newly-added layers. If mapControllerRef.current exposes a multi-layer fit method (or one can be added), calling it after the loop would resolve this. Otherwise, falling back to the first layer rather than the last would be less surprising as a convention.
| if (extension === "gpx") { | ||
| layers.push(...parseGpxTextLayers(await file.text(), file.name)); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling (medium confidence): The Tauri loadDroppedVectorPaths path (added in this same PR) wraps GPX parse errors with "Could not read this GPX file. ${detail}" before rethrowing. The browser path here does not — a parse error from parseGpxLayer (e.g. "The GPX file is not valid XML." or "No valid GPX waypoints, routes, or tracks were found.") propagates raw to whatever catch block the caller has.
For parity you could wrap the same way:
| if (extension === "gpx") { | |
| layers.push(...parseGpxTextLayers(await file.text(), file.name)); | |
| continue; | |
| } | |
| if (extension === "gpx") { | |
| try { | |
| layers.push(...parseGpxTextLayers(await file.text(), file.name)); | |
| } catch (error) { | |
| const detail = error instanceof Error ? error.message : "Unknown error"; | |
| throw new Error(`Could not read this GPX file. ${detail}`); | |
| } | |
| continue; | |
| } |
| function parseGpxTextLayers(text: string, path: string): LoadedVectorLayer[] { | ||
| const result = parseGpxLayer(text); | ||
| const baseName = pathWithoutExtension(browserSafeFileName(path)) || "GPX"; | ||
| return [ | ||
| { data: result.waypoints, label: "Waypoints" }, | ||
| { data: result.tracks, label: "Tracks" }, | ||
| { data: result.routes, label: "Routes" }, | ||
| ] | ||
| .filter((layer) => layer.data.features.length > 0) | ||
| .map((layer) => ({ | ||
| data: layer.data, | ||
| name: `${baseName} ${layer.label}`, | ||
| path, | ||
| })); |
There was a problem hiding this comment.
Nits (grouped, low confidence):
-
The intermediate objects use a
labelproperty that has no declared type and is immediately consumed in.map()to producename. TypeScript infers this fine, but the intent can be made clearer with a typed tuple or a simpleconst LAYERSwith an explicit type annotation. -
loadTauriVectorFilekeeps its original return typePromise<{ data: FeatureCollection; path: string }>even thoughLoadedVectorLayernow exists. Structurally it's compatible (thename?field is optional), but updating the annotation would document intent and unify the codebase.
Neither is a correctness issue, just readability.
Code reviewBugs / UX
PerformanceNothing found. SecurityNothing found. Quality
CLAUDE.mdNo Summary: The logic is sound and the split-layer approach is well-structured. The main actionable concern is the |
Summary
Test plan