fix(import): preserve QGIS source paths and group visibility - #1584
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe QGIS importer now inherits visibility from unchecked ancestor groups. It also normalizes quoted paths and file URLs, handles malformed file URLs, preserves HTTP query components, and tests remote URL and nested-group behavior. ChangesQGIS import updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/lib/qgis-project-import.ts`:
- Line 471: Move the quote normalization in the source-processing flow before
the `/vsicurl` prefix removal so quoted remote URLs match the virtual-prefix
expression and retain their query string. Update the relevant logic around
source handling and add a regression test covering a quoted
`/vsicurl/https://...?...` remote source.
In `@tests/qgis-project-import.test.ts`:
- Around line 275-278: Update the test assertion near the existing unc.warnings
check to also assert that unc.project.layers is empty, verifying the UNC layer
is excluded while preserving the current network-path warning assertion.
🪄 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: fc5fb7b5-ccb7-4a51-8d57-8d7ce4d4590d
📒 Files selected for processing (2)
apps/geolibre-desktop/src/lib/qgis-project-import.tstests/qgis-project-import.test.ts
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 (2)
tests/qgis-project-import.test.ts (1)
282-296: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the nested layer visibility.
The test asserts that the parent and nested groups are hidden, but it does not assert the flattened
citieslayer visibility. A regression could leave that layer visible while the group metadata is hidden.Assert that the imported
citieslayer hasvisible: falsewhen theTransportancestor is unchecked.🤖 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/qgis-project-import.test.ts` around lines 282 - 296, Extend the test in “inherits visibility from unchecked parent groups” to also locate the imported cities layer in result.project and assert its visible property is false when the Transport ancestor is unchecked, while preserving the existing group visibility assertions.apps/geolibre-desktop/src/lib/qgis-project-import.ts (1)
472-486: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve encoded query and fragment characters in file paths.
Line 475 decodes
%3Fand%23into?and#. Line 486 then treats those characters as query or fragment delimiters and truncates the filename. For example,file:///tmp/a%23b.geojsonbecomes/tmp/a.Skip generic query/fragment stripping for successfully decoded file URLs, and add a regression test for an encoded
#filename.🤖 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/qgis-project-import.ts` around lines 472 - 486, Track whether the file URL was successfully parsed and decoded in the file-source normalization block, then skip the generic query/fragment removal after that path when the flag is set. Preserve stripping for non-file sources and fallback parsing, and add a regression test covering an encoded “#” filename such as file:///tmp/a%23b.geojson.
🤖 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/lib/qgis-project-import.ts`:
- Around line 472-486: Track whether the file URL was successfully parsed and
decoded in the file-source normalization block, then skip the generic
query/fragment removal after that path when the flag is set. Preserve stripping
for non-file sources and fallback parsing, and add a regression test covering an
encoded “#” filename such as file:///tmp/a%23b.geojson.
In `@tests/qgis-project-import.test.ts`:
- Around line 282-296: Extend the test in “inherits visibility from unchecked
parent groups” to also locate the imported cities layer in result.project and
assert its visible property is false when the Transport ancestor is unchecked,
while preserving the existing group visibility assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6edf6882-dce0-466a-a9f8-714bb5a4f16b
📒 Files selected for processing (2)
apps/geolibre-desktop/src/lib/qgis-project-import.tstests/qgis-project-import.test.ts
Member
|
/claude-review |
Contributor
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Quality / Correctness verification
Performance / CLAUDE.md
|
giswqs
added a commit
that referenced
this pull request
Jul 31, 2026
#1593 changed QGIS layer-group names from flattened ("Transport / Places") to plain names plus a parentId. #1584 branched before that change and added a visibility-inheritance test still asserting the flattened name, so main went red once both landed. The importer behavior is correct; only the expectation was stale.
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
2 participants
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.
Summary
file://server/share/...sources so network paths are rejected explicitlyRoot cause
The importer normalized remote URLs with the same query stripping used for local file options, discarded the hostname while converting file URLs, and stored only each flattened group's own checked state. Those transformations could break authenticated fetches, treat UNC sources as local paths, and show layers beneath a hidden parent group.
Validation
node --import tsx --test tests/qgis-project-import.test.ts(13 passed)cargo check --manifest-path apps/geolibre-desktop/src-tauri/Cargo.tomlSummary by CodeRabbit
Bug Fixes
Tests