feat: add KML and KMZ layer export - #1568
Conversation
|
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:
📝 WalkthroughWalkthroughGeoLibre adds native KML and KMZ layer export. GeoJSON features are serialized with geometry, properties, metadata, and styles; KMZ packages the generated KML as ChangesKML and KMZ export
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LayerPanel
participant exportVectorLayer
participant exportTextLayer
participant exportBinaryVectorLayer
participant writeKml
participant exportKmz
LayerPanel->>exportVectorLayer: select KML
exportVectorLayer->>exportTextLayer: route text export
exportTextLayer->>writeKml: serialize FeatureCollection
writeKml-->>exportTextLayer: KML document
LayerPanel->>exportBinaryVectorLayer: select KMZ
exportBinaryVectorLayer->>exportKmz: create KMZ
exportKmz->>writeKml: serialize FeatureCollection
writeKml-->>exportKmz: doc.kml content
exportKmz-->>exportBinaryVectorLayer: zipped KMZ bytes
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lib/vector-export.ts`:
- Around line 181-221: Refactor exportTextLayer to derive the format-specific
extension, label, MIME type, file filters, and browser accept mapping from one
lookup table keyed by format, instead of repeating isCsv/isKml ternary chains.
Reuse the table’s MIME and extension values for both mimeType and
browserTypes[0].accept so each format’s metadata has a single source of truth.
In `@tests/kml-writer.test.ts`:
- Around line 15-26: Extend the KML writer test fixture and assertions around
SAMPLE to cover stroke, stroke-width, and fill properties. Verify that styleKml
emits the expected LineStyle width and PolyStyle color values, while preserving
the existing IconStyle coverage.
- Around line 128-138: Add a test in the KML export suite covering
exportVectorLayer/exportTextLayer with the "kml" format, asserting the generated
text, extension, label, MIME type, filters, and browser-save accept
configuration used by vector-export.ts. Reuse the existing SAMPLE fixture and
expected writeKml output, and keep the existing KMZ coverage unchanged.
🪄 Autofix (Beta)
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: 1d815f92-5bf5-423a-9a91-bb63945fd6a0
📒 Files selected for processing (6)
apps/geolibre-desktop/src/components/panels/AttributeTable.tsxapps/geolibre-desktop/src/components/panels/LayerPanel.tsxapps/geolibre-desktop/src/lib/kml-writer.tsapps/geolibre-desktop/src/lib/vector-export.tsapps/geolibre-desktop/src/lib/vector-exporter.tstests/kml-writer.test.ts
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 `@tests/kml-writer.test.ts`:
- Around line 150-169: Restore the original globalThis.self value in the
browser-picker test’s cleanup alongside globalThis.window. Capture whether self
existed and its prior value before assigning it, then have the finally block
remove self when it was originally absent or restore the saved value when it
existed.
🪄 Autofix (Beta)
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: f7fafd8d-40b6-4233-9683-36e1d9941629
📒 Files selected for processing (2)
apps/geolibre-desktop/src/lib/vector-export.tstests/kml-writer.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-scoped, well-tested addition (geometry types, escaping, invalid-coordinate handling, KMZ packaging, and the browser save picker are all covered in |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Also checked and found no issues: XML escaping order/control-character handling, KML color byte ordering (verified against the added unit tests), polygon ring closing, GeometryCollection/Multi* recursion, error handling around |
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 (1)
tests/kml-writer.test.ts (1)
163-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the feature-ID error path.
The new writer branch at Line 216 uses
feature.idwhen present, but this test only exercises the index fallback. Add a malformed feature with an ID and assert the error containsfeature with id ...so this new diagnostic cannot regress unnoticed.🤖 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 `@tests/kml-writer.test.ts` around lines 163 - 181, The invalid-coordinate test in the KML writer should also cover the feature-ID diagnostic path. Add a malformed feature with a defined ID to the test suite and assert that writeKml throws an error containing the expected “feature with id …” context, while preserving the existing index-based assertion.
🤖 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 `@tests/kml-writer.test.ts`:
- Around line 163-181: The invalid-coordinate test in the KML writer should also
cover the feature-ID diagnostic path. Add a malformed feature with a defined ID
to the test suite and assert that writeKml throws an error containing the
expected “feature with id …” context, while preserving the existing index-based
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 12e28595-3e97-43fc-8799-14bc614b37f6
📒 Files selected for processing (2)
apps/geolibre-desktop/src/lib/kml-writer.tstests/kml-writer.test.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the KML/KMZ writer is careful and well-tested (escaping, altitude handling, ring closing, per-feature error isolation with clear messages, KMZ packaging), and the export wiring reuses existing patterns cleanly. |
|
Addressed the latest CodeRabbit follow-up in f5f1491. The invalid-coordinate test now covers both the index fallback and the feature-ID diagnostic path. |
Code reviewBugs: None found. Traced the full data path ( Security: None found. All text (names, descriptions, property keys/values, document name) is passed through Performance: None found. KML/KMZ writer modules are lazy-loaded via dynamic Quality: Low-confidence nit posted inline — the new coordinate-validation error message and its feature-context wrapper ( CLAUDE.md: No other adherence issues found. The new "KML"/"KMZ" menu labels are hardcoded like their sibling format labels ("GeoJSON", "GeoPackage", "Shapefile (zipped)") in the same dropdowns, so they're consistent with existing (untranslated) file convention rather than a new violation. |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Code reviewBugs: None found. Security: None found. Property values and keys are XML-escaped via Performance: None found. Building the KML document in memory mirrors the existing GeoJSON/CSV exporters' approach; no new regression introduced. Quality:
CLAUDE.md: No violations. New i18n keys were correctly added only to Overall this is a well-implemented, well-tested feature (291 new lines in |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Code reviewBugs: None found. Coordinate/geometry construction ( Security: No injection issues. All XML-facing content ( Performance: No concerns; string building is linear in feature/geometry count and consistent with the existing GeoJSON/CSV writers. Confidence: high. Quality:
CLAUDE.md: No violations. New user-facing strings go through |
Summary
doc.kmlarchive entryTesting
npm run buildnpm run test:frontendpre-commit run --all-filesus_cities.geojsonto KML in light mode and KMZ in dark mode, then imported both files back into GeoLibreFixes #1566
Summary by CodeRabbit