feat: add DGGS plugin - #1650
Merged
Merged
Conversation
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:
📝 WalkthroughWalkthroughThe PR adds seven DGGS MapLibre plugins, extends H3 controls and geometry handling, updates public exports and dependencies, integrates DGGS plugins into the desktop menu, and adds localization, viewer-mode restrictions, and helper tests. ChangesDGGS plugin expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant PluginsMenu
participant DGGSPlugin
participant MapLibre
User->>PluginsMenu: Open the Plugins menu
PluginsMenu->>DGGSPlugin: Render the grouped DGGS submenu
User->>DGGSPlugin: Activate a grid or identify a cell
DGGSPlugin->>MapLibre: Update grid and overlay sources
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Member
|
/claude-review |
Contributor
|
I'll stop here and wait — the four background agents reviewing the new plugin files will notify me when finished. |
![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: 18
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/components/layout/TopToolbar.tsx (1)
967-994: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the
groupIdprefill inTopToolbar.
LayerPanelopens dialog-based sources withopenAddData(..., { groupId: group.id }), andOpenAddDataDetail.groupIddocuments that these layers should be scoped to that group.onOpenAddDatanow ignoresdetail.groupIdandAddDataDialogdoes not receive/proxy it, so the "Add data to group" entries no longer add the created layer into the selected group. Map the window-event group prefill into a dialog prop and expose it to sources that manage group assignment.🤖 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/components/layout/TopToolbar.tsx` around lines 967 - 994, Restore groupId propagation in the add-data flow: update TopToolbar’s onOpenAddData to retain detail.groupId, pass it through the AddDataDialog props, and expose it to the relevant source components so layer creation remains scoped to the selected group. Preserve existing PostgreSQL prefill and source-kind handling.
🤖 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/plugins/src/plugins/maplibre-dggal.ts`:
- Around line 328-331: Update the dggrsType validation in the candidate
resolution flow to accept only keys owned by DGGAL_TYPES, replacing the
inherited-key-prone in check while preserving the existing string guard and
default fallback. Ensure prototype names such as constructor and toString are
rejected before activeDggrs() uses the value.
- Around line 459-483: Update dggalGridForBounds to handle bounds where east is
less than west by normalizing or splitting the antimeridian-crossing interval,
following the approach used in maplibre-dggrid.ts. Ensure the area estimate and
limit guard use the correct covered width, and pass listZones longitude bounds
in a valid order so crossing views produce a populated grid.
In `@packages/plugins/src/plugins/maplibre-dggrid.ts`:
- Around line 509-541: Update ringIntersectsBounds to normalize the entire ring
once into a consistent longitude frame relative to the bounds window, preserving
continuity for antimeridian-crossing vertices. Reuse that normalized ring for
the vertex checks, pointInRing corner checks, and segmentCrossesLonLatRect calls
instead of independently normalizing vertices or passing the raw ring.
- Line 837: Replace the full renderPanel call in refresh() at
packages/plugins/src/plugins/maplibre-dggrid.ts:837 with targeted updates to the
status text and resolution slider/value, preserving existing controls and user
input. Apply the same narrow-update change in
packages/plugins/src/plugins/maplibre-dggal.ts:618 instead of rebuilding the
panel.
- Around line 216-223: Both loader promises permanently cache failures; update
loadDggrid in packages/plugins/src/plugins/maplibre-dggrid.ts (lines 216-223)
and loadDggal in packages/plugins/src/plugins/maplibre-dggal.ts (lines 205-210)
to append rejection handlers that reset dggsPromise and dggalPromise to null
respectively before rethrowing the original error, preserving retry behavior
after dynamic import or WASM initialization failures.
- Around line 674-700: Replace the queue.shift() operation in the BFS loop with
an index cursor that advances through queue entries, and update the loop
condition to use that cursor instead of queue.length. Preserve the existing
traversal, coverage checks, maxPops guard, and neighbor enqueuing behavior.
- Around line 1214-1232: Prevent async activation from completing after
deactivation by adding a generation token in the activate/deactivate flows of
packages/plugins/src/plugins/maplibre-dggrid.ts (lines 1214-1232) and
packages/plugins/src/plugins/maplibre-dggal.ts (lines 991-1010): capture the
current token before awaiting loadDggrid/loadDggal, return false if the token
changed after the await, and increment the token in each plugin’s deactivate so
stale activations attach no listeners, panels, or layers.
In `@packages/plugins/src/plugins/maplibre-geohash.ts`:
- Around line 188-193: Update wrapLongitude to return the normalized wrapped
calculation directly, removing the redundant ternary checks for -180 and 180
while preserving the existing [-180, 180) range.
In `@packages/plugins/src/plugins/maplibre-h3.ts`:
- Around line 297-310: Extract the shared -130 longitude heuristic into a named
constant such as TRANSMERIDIAN_LONGITUDE_THRESHOLD, then replace the literal in
h3FixTransmeridianBoundary and the corresponding parentCells check with that
constant so both functions use the same threshold.
- Around line 18-26: Update ICOSAHEDRON_GEOJSON_URL to reference a specific
immutable commit SHA or stable release tag for the upstream icosahedron.geojson
resource instead of the mutable main branch, preserving the existing file path
and URL host.
- Around line 545-573: Export the parentCells function so it can be tested
externally, then add unit tests in the existing H3 plugin test suite covering a
non-dateline cell’s canonical parent and any limited overlapping neighbors,
resolution-0 cells returning [], and a dateline-crossing cell exercising the
ringCenterLng adjustment.
In `@packages/plugins/src/plugins/maplibre-olc.ts`:
- Around line 370-384: Update olcGridForBounds to deduplicate generated cells
using a Set keyed by the same `${cell}@${lngOffset}` identifier pattern as
geohashGridForBounds. Skip already-seen OLC/world-copy pairs before pushing the
feature, while preserving the existing limit check for unique features.
In `@packages/plugins/src/plugins/maplibre-s2.ts`:
- Around line 260-300: Update the RegionCoverer creation to set an explicit
maxCells value larger than the viewport cell limit, while retaining the existing
minLevel and maxLevel configuration. Locate the coverer initialization in the
map rendering flow and ensure the configured limit prevents the default S2 cap
from truncating required coverage.
In `@tests/a5-plugin.test.ts`:
- Around line 28-47: Update the A5 test expectations in the resolution-settings
assertion and the a5ResolutionForZoom test to derive the maximum resolution from
MAX_RESOLUTION instead of hardcoding 30. Import or re-export MAX_RESOLUTION
through the plugin module as needed to use the same dependency version as the
implementation, while leaving the fixed S2 level tests unchanged.
- Around line 57-68: Extend the “creates export-ready polygon features” test
around a5CellFeature to assert the polygon ring’s final coordinate equals its
first coordinate, confirming closure rather than only checking coordinate shape.
Add an antimeridian-crossing A5 feature test, following the S2 suite’s
equivalent case, and verify the returned ring remains longitude-contiguous
without requiring longitude unwrapping.
In `@tests/dggal-plugin.test.ts`:
- Around line 47-50: Add a validation assertion alongside the existing
"NotAGrid" case in the grid-type normalization test, using an inherited
Object.prototype key such as "toString" and verifying it falls back to
DEFAULT_DGGAL_GRID_SETTINGS.dggrsType. Keep the test focused on
normalizeDggalGridSettings and preserve the existing invalid-value assertion.
In `@tests/dggrid-plugin.test.ts`:
- Around line 102-106: Strengthen the antimeridian test in the `supports
antimeridian-crossing bounds` case by adding the same ring-span/contiguity
assertion used by the equivalent DGGAL test and asserting a reasonable upper
bound on `grid.features.length`. Keep the existing positive-count check and use
the requested antimeridian bounds to ensure `ringIntersectsBounds` does not
over-collect cells near ±180.
In `@tests/olc-plugin.test.ts`:
- Around line 142-163: Add a test assertion in the existing “finds the single
parent, child count, and neighbors of a cell” case that encodes a
grid-refinement cell at code length 11 and verifies olcParentCell returns its
expected code-length-10 parent, covering the refinement-to-pair transition in
OLC_CODE_LENGTHS.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx`:
- Around line 967-994: Restore groupId propagation in the add-data flow: update
TopToolbar’s onOpenAddData to retain detail.groupId, pass it through the
AddDataDialog props, and expose it to the relevant source components so layer
creation remains scoped to the selected group. Preserve existing PostgreSQL
prefill and source-kind handling.
🪄 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: ddd25891-5bd9-41e4-96f0-7d03142ff6eb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/components/layout/toolbar/PluginsMenu.tsxapps/geolibre-desktop/src/hooks/usePlugins.tsapps/geolibre-desktop/src/i18n/locales/en.jsonpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/dggs-group.tspackages/plugins/src/plugins/maplibre-a5.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tspackages/plugins/src/plugins/maplibre-geohash.tspackages/plugins/src/plugins/maplibre-h3.tspackages/plugins/src/plugins/maplibre-olc.tspackages/plugins/src/plugins/maplibre-s2.tspackages/plugins/src/plugins/maplibre-tilecode.tstests/a5-plugin.test.tstests/dggal-plugin.test.tstests/dggrid-plugin.test.tstests/geohash-plugin.test.tstests/h3-plugin.test.tstests/olc-plugin.test.tstests/s2-plugin.test.tstests/tilecode-plugin.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx (2)
549-786: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting a shared label-builder for the DGGS grid plugins.
The blocks for H3, S2, A5, DGGRID, DGGAL, OLC, Geohash, and Tilecode repeat about 20 identical field names (
title,getTitle,controlTitle,resolution,cellCount,tooManyCells,fillColor,fillOpacity,lineColor,lineWidth,showLabels,identifyHint,selectedCell,noSelection,copyId,children,neighbors,center,zoomToCell,addAsLayer,exportGeoJson,exportCsv,includeNeighbors). Only a few fields differ per plugin (gridType/cellType,quadkey,includeParent/includeParents).Extract a helper that builds the common label object from an i18n key prefix, then merge plugin-specific fields before calling the typed setter. This reduces about 200 lines of near-duplicate code and makes future DGGS additions less error-prone.
🤖 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/components/layout/TopToolbar.tsx` around lines 549 - 786, Extract a shared label-builder near the repeated setH3Labels, setS2Labels, setA5Labels, setDggridLabels, setDggalLabels, setOlcLabels, setGeohashLabels, and setTilecodeLabels calls that accepts each plugin’s i18n key prefix and returns the common translated fields, including dynamic cell-count handlers and getTitle. Replace the duplicated common objects with the helper, then merge each plugin’s unique fields such as gridType, cellType, quadkey, and includeParent/includeParents before passing them to the existing typed setters.
175-179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign toolbar viewer gating with the removed
TopToolbarProps.viewerprop.
DesktopShell.tsxstill passesviewer={layoutOptions.viewer}toTopToolbar, but the prop no longer exists. IfTopToolbarshould not gate menus/dialogs by viewer mode, remove the prop from both the caller and anyviewer-aware toolbar prop types; if it should, add a local read-only flag that flows into the relevant toolbar menus and dialogs.🤖 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/components/layout/TopToolbar.tsx` around lines 175 - 179, Align TopToolbar usage with the removed viewer prop: update DesktopShell.tsx to stop passing viewer={layoutOptions.viewer}, and remove any remaining viewer-aware fields or gating from TopToolbar prop types and its menus/dialogs. Preserve toolbar behavior without viewer-mode checks.
♻️ Duplicate comments (1)
packages/plugins/src/plugins/maplibre-olc.ts (1)
370-384: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDedupe OLC cells during the viewport walk.
olcGridForBoundsstepslng/latwith floating-point accumulation. Two adjacent steps can land inside the same OLC cell, so the same cell and world copy can be pushed twice. Duplicate features share the sameid, which double-paints the fill and breaks the uniqueness assertion intests/olc-plugin.test.tsat line 75.geohashGridForBoundsinpackages/plugins/src/plugins/maplibre-geohash.tsalready guards this with a${cell}@${lngOffset}key.🐛 Proposed fix
const features: Feature<Polygon>[] = []; + const seen = new Set<string>(); for (let lng = startLng; lng < east; lng += lngWidth) { for (let lat = startLat; lat < north && lat < 90; lat += latHeight) { const centerLng = lng + lngWidth / 2; const cell = OpenLocationCode.encode(lat + latHeight / 2, centerLng, codeLength); // 360° multiple between the drawn column and the normalized cell. const lngOffset = Math.round((centerLng - OpenLocationCode.decode(cell).longitudeCenter) / 360) * 360; + const key = `${cell}@${lngOffset}`; + if (seen.has(key)) continue; + seen.add(key); features.push(olcCellFeature(cell, lngOffset));🤖 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/plugins/src/plugins/maplibre-olc.ts` around lines 370 - 384, Update olcGridForBounds to deduplicate generated features using a key combining each encoded cell and its normalized lngOffset, matching the guard used by geohashGridForBounds. Check and record the key before pushing via olcCellFeature, while preserving the existing limit enforcement and viewport traversal.
🤖 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/i18n/locales/en.json`:
- Around line 2894-2903: Update the S2 locale entries `parent` and
`includeParents` to use the plural “Parent(s)” and “Include selected cell
parent(s)” wording consistent with sibling multi-parent DGGS plugins; only
rename `includeParents` to `includeParent` if the S2 implementation genuinely
supports a single parent level.
In `@packages/plugins/package.json`:
- Line 65: Align the maplibre-gl-vector dependency across
packages/plugins/package.json, apps/geolibre-desktop/package.json, and
package-lock.json by using the ^0.10.7 range where supported. Update the
lockfile’s package entry and resolved metadata to match the declared 0.10.7
range, preserving a consistent dependency version across all manifests.
In `@packages/plugins/src/plugins/maplibre-a5.ts`:
- Around line 281-291: Normalize the longitude values returned by cellToBoundary
in a5CellFeature before assigning them to geometry.coordinates[0], unwrapping
the ring across the antimeridian relative to adjacent points as the S2 helper
does. Ensure fitSelected uses the same normalized boundary coordinates when
computing bounds, so antimeridian-spanning cells render and fit locally rather
than across the globe.
In `@packages/plugins/src/plugins/maplibre-h3.ts`:
- Around line 442-458: Update ensureLayers() so the ICOSAHEDRON_SOURCE_ID source
and ICOSAHEDRON_LINE_LAYER_ID layer are created only when
settings.showIcosahedron is true, preventing the hidden overlay’s GeoJSON
request. Also guard the corresponding setLayoutProperty() and removeLayers()
operations so they run only when the icosahedron layer exists.
In `@tests/dggrid-plugin.test.ts`:
- Around line 138-144: Update the “returns every overlapping direct parent,
canonical first” test to explicitly call setDggs with DGGRID_CONFIG immediately
after loading the shared engine and before performing any parent lookups; import
DGGRID_CONFIG alongside the existing test imports so the test always uses the
default ISEA4H configuration.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx`:
- Around line 549-786: Extract a shared label-builder near the repeated
setH3Labels, setS2Labels, setA5Labels, setDggridLabels, setDggalLabels,
setOlcLabels, setGeohashLabels, and setTilecodeLabels calls that accepts each
plugin’s i18n key prefix and returns the common translated fields, including
dynamic cell-count handlers and getTitle. Replace the duplicated common objects
with the helper, then merge each plugin’s unique fields such as gridType,
cellType, quadkey, and includeParent/includeParents before passing them to the
existing typed setters.
- Around line 175-179: Align TopToolbar usage with the removed viewer prop:
update DesktopShell.tsx to stop passing viewer={layoutOptions.viewer}, and
remove any remaining viewer-aware fields or gating from TopToolbar prop types
and its menus/dialogs. Preserve toolbar behavior without viewer-mode checks.
---
Duplicate comments:
In `@packages/plugins/src/plugins/maplibre-olc.ts`:
- Around line 370-384: Update olcGridForBounds to deduplicate generated features
using a key combining each encoded cell and its normalized lngOffset, matching
the guard used by geohashGridForBounds. Check and record the key before pushing
via olcCellFeature, while preserving the existing limit enforcement and viewport
traversal.
🪄 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: f9f6543c-b88c-4e3d-aae3-7b8739756f25
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/components/layout/toolbar/PluginsMenu.tsxapps/geolibre-desktop/src/hooks/usePlugins.tsapps/geolibre-desktop/src/i18n/locales/en.jsonpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/dggs-group.tspackages/plugins/src/plugins/maplibre-a5.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tspackages/plugins/src/plugins/maplibre-geohash.tspackages/plugins/src/plugins/maplibre-h3.tspackages/plugins/src/plugins/maplibre-olc.tspackages/plugins/src/plugins/maplibre-s2.tspackages/plugins/src/plugins/maplibre-tilecode.tstests/a5-plugin.test.tstests/dggal-plugin.test.tstests/dggrid-plugin.test.tstests/geohash-plugin.test.tstests/h3-plugin.test.tstests/olc-plugin.test.tstests/s2-plugin.test.tstests/tilecode-plugin.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
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 `@packages/plugins/src/plugins/maplibre-dggrid.ts`:
- Around line 675-680: Update the BFS traversal surrounding maxPops and the loop
in the viewport collection function to throw a RangeError after the loop when
head remains less than queue.length. Preserve the existing collected return path
when the queue is fully processed.
🪄 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: 81e5091f-ad72-4d42-aa0b-5bb17bb34061
📒 Files selected for processing (4)
apps/geolibre-desktop/src/i18n/locales/en.jsonpackages/plugins/src/plugins/maplibre-dggrid.tstests/a5-plugin.test.tstests/dggal-plugin.test.ts
Contributor
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
|
Member
|
@thangqd Merge is still blocked by CodeRabbit. Could you fix the remaining comments or allow maintainer' edits so that I can fix them? Thanks. |
Contributor
Author
|
@giswqs CodeRabbit requested changes have been fixed. Please review when convenient. Thanks! |
Member
|
CodeRabbit still blocked it as there are some unresolved comments. |
Contributor
|
✅ Action performedReviews resumed. Review finished.
|
Contributor
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 (2)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx (2)
1046-1062: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock project mutations in viewer mode.
The global
OPEN_ADD_DATA_EVENThandler still opensAddDataDialogin viewer mode. The project-name input also remains editable and callssetProjectName. A read-only embed can therefore add layers or modify the project name.
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx#L1046-L1062: Return before processingOPEN_ADD_DATA_EVENTwhenvieweris true. Includeviewerin the effect dependencies.apps/geolibre-desktop/src/components/layout/TopToolbar.tsx#L1708-L1728: Make the project-name input read-only, or do not render it, whenvieweris true.🤖 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/components/layout/TopToolbar.tsx` around lines 1046 - 1062, Block project mutations in viewer mode: in the OPEN_ADD_DATA_EVENT handler within the relevant useEffect, return before processing when viewer is true and include viewer in the effect dependencies; at apps/geolibre-desktop/src/components/layout/TopToolbar.tsx lines 1046-1062 apply this handler change. At lines 1708-1728, make the project-name input read-only or omit it when viewer is true so it cannot call setProjectName.
1580-1587: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize the command title.
"View Comments"is user-facing text. Uset()and add the source key toapps/geolibre-desktop/src/i18n/locales/en.json.As per coding guidelines,
apps/geolibre-desktop/src/**/*.{ts,tsx}must uset()for new user-facing UI strings.🤖 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/components/layout/TopToolbar.tsx` around lines 1580 - 1587, Update the view.comments command in TopToolbar to obtain its title through t() instead of the hardcoded “View Comments” string, and add the corresponding source translation key and English value to the locale definitions in en.json.Source: Coding guidelines
🤖 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/plugins/src/plugins/maplibre-dggrid.ts`:
- Around line 863-877: Keep the automatic-resolution display synchronized during
status-only refreshes: in packages/plugins/src/plugins/maplibre-dggrid.ts lines
863-877, tag the resolution input and value element in renderPanel and update
both with effectiveResolution() from updatePanelStatus when
settings.autoResolution is enabled; apply the same targeted update in
packages/plugins/src/plugins/maplibre-dggal.ts lines 639-653. Also revise the
refresh comments in maplibre-dggrid.ts lines 1057-1059 and maplibre-dggal.ts
lines 827-829 to no longer claim the panel is fully re-rendered.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/TopToolbar.tsx`:
- Around line 1046-1062: Block project mutations in viewer mode: in the
OPEN_ADD_DATA_EVENT handler within the relevant useEffect, return before
processing when viewer is true and include viewer in the effect dependencies; at
apps/geolibre-desktop/src/components/layout/TopToolbar.tsx lines 1046-1062 apply
this handler change. At lines 1708-1728, make the project-name input read-only
or omit it when viewer is true so it cannot call setProjectName.
- Around line 1580-1587: Update the view.comments command in TopToolbar to
obtain its title through t() instead of the hardcoded “View Comments” string,
and add the corresponding source translation key and English value to the locale
definitions in en.json.
🪄 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: f89e75a9-14eb-4f6f-873c-4230957f3d8a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/maplibre-a5.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tspackages/plugins/src/plugins/maplibre-olc.tstests/dggrid-plugin.test.tstests/olc-plugin.test.ts
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 `@packages/plugins/src/plugins/maplibre-dggal.ts`:
- Around line 636-654: Update the comment immediately above the
updatePanelStatus() call in the pan/zoom handling logic to state that panel
status is updated without rebuilding the whole panel, preserving open color
pickers and focused inputs during gestures.
In `@packages/plugins/src/plugins/maplibre-dggrid.ts`:
- Around line 1251-1262: Update activate to wrap the loadDggrid() await in a
try/catch. When loading fails, set currentError only if the generation counter
still matches, then return false so activation failures do not reject; preserve
the existing generation guard and failure-panel flow.
🪄 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: f89e75a9-14eb-4f6f-873c-4230957f3d8a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/maplibre-a5.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tspackages/plugins/src/plugins/maplibre-olc.tstests/dggrid-plugin.test.tstests/olc-plugin.test.ts
CodeRabbit still requested try/catch around activate loads and comments that matched updatePanelStatus instead of a full rebuild.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
giswqs
enabled auto-merge (squash)
August 3, 2026 18:44
giswqs
disabled auto-merge
August 3, 2026 18:44
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/hooks/usePlugins.ts (1)
39-39: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore ArcGIS Hub, CKAN, and Socrata in the desktop plugin load path.
usePlugins.tsno longer imports or registers those plugins, whilepackages/plugins/src/index.tsstill exports them. Either add the consolidated Open Data Catalog plugin here and keep the exports consistent, or document/remove this as intentional deprecation.🤖 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/usePlugins.ts` at line 39, Update usePlugins to import and register the consolidated Open Data Catalog plugin so ArcGIS Hub, CKAN, and Socrata are restored in the desktop load path, keeping the registration consistent with the exports in packages/plugins/src/index.ts.
♻️ Duplicate comments (1)
tests/dggrid-plugin.test.ts (1)
145-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the DGGS configuration before the parent lookup.
loadDggridreturns one shared engine, anddggridGridForBoundscallsengine.setDggs(...). This test depends on the configuration left by the earlier tests. The preceding limit test happens to restore the defaultDGGRID_CONFIG, so the assertion passes today, but the result depends on test order.Call
setDggsexplicitly at the start of this test.♻️ Proposed fix
it("returns every overlapping direct parent, canonical first", async () => { const engine = await loadDggrid(); + // Other tests reconfigure the shared engine, so pin the DGGS here. + engine.setDggs({ ...DGGRID_CONFIG }, 5); const [cell] = engine.geoToSequenceNum([[106.6, 10.8]], 5);Add
DGGRID_CONFIGto the import list at Lines 3-13.🤖 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/dggrid-plugin.test.ts` around lines 145 - 151, Update the test “returns every overlapping direct parent, canonical first” to explicitly call the shared engine’s setDggs with DGGRID_CONFIG before performing the parent lookup, and add DGGRID_CONFIG to the existing imports. Keep the current parent assertions unchanged.
🤖 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/plugins/src/plugins/maplibre-dggal.ts`:
- Around line 464-488: Normalize west and east with the existing normalizeLon
pattern at the start of dggalGridForBounds, before the east < west antimeridian
check and full-world span handling. Ensure refresh inputs outside ±180 produce
longitudes within [-180, 180] before any listZones bounding box is constructed.
In `@packages/plugins/src/plugins/maplibre-dggrid.ts`:
- Around line 1343-1357: Update applyProjectState in
packages/plugins/src/plugins/maplibre-dggrid.ts at lines 1343-1357 to call
renderPanel(panelContainer) after refresh() when panelContainer is set, keeping
restored cell type, projection, aperture, color, and checkbox controls
synchronized. Apply the same change in
packages/plugins/src/plugins/maplibre-dggal.ts at lines 1121-1131 so the DGGS
type select and resolution slider reflect restored settings.
In `@tests/dggal-plugin.test.ts`:
- Around line 113-124: Extend the antimeridian bounds coverage around
dggalGridForBounds with a case using wrapped bounds where east is less than
west, matching the form used by tests/dggrid-plugin.test.ts. Assert the returned
cells are present and remain deduplicated, so the east < west split and merge
behavior are exercised.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/hooks/usePlugins.ts`:
- Line 39: Update usePlugins to import and register the consolidated Open Data
Catalog plugin so ArcGIS Hub, CKAN, and Socrata are restored in the desktop load
path, keeping the registration consistent with the exports in
packages/plugins/src/index.ts.
---
Duplicate comments:
In `@tests/dggrid-plugin.test.ts`:
- Around line 145-151: Update the test “returns every overlapping direct parent,
canonical first” to explicitly call the shared engine’s setDggs with
DGGRID_CONFIG before performing the parent lookup, and add DGGRID_CONFIG to the
existing imports. Keep the current parent assertions 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: c0b5a6b7-a7b1-40da-9bf9-477f0ad0a3de
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/components/layout/toolbar/PluginsMenu.tsxapps/geolibre-desktop/src/hooks/usePlugins.tsapps/geolibre-desktop/src/i18n/locales/en.jsonpackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/plugins/dggs-group.tspackages/plugins/src/plugins/maplibre-a5.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tspackages/plugins/src/plugins/maplibre-geohash.tspackages/plugins/src/plugins/maplibre-h3.tspackages/plugins/src/plugins/maplibre-olc.tspackages/plugins/src/plugins/maplibre-s2.tspackages/plugins/src/plugins/maplibre-tilecode.tstests/a5-plugin.test.tstests/dggal-plugin.test.tstests/dggrid-plugin.test.tstests/geohash-plugin.test.tstests/h3-plugin.test.tstests/olc-plugin.test.tstests/s2-plugin.test.tstests/tilecode-plugin.test.ts
Re-register ArcGIS Hub/CKAN/Socrata and fetchVectorUrl, normalize DGGAL longitudes, refresh panels after project restore, and cover wrapped antimeridian bounds.
Contributor
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)
packages/plugins/src/plugins/maplibre-dggal.ts (1)
471-497: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve full-world longitude spans before endpoint normalization.
For bounds such as
[0, south, 360, north], Lines 472-473 normalize both endpoints to0. Line 494 then sees a zero-width viewport instead of a full-world viewport.listZones()can return an empty or incomplete grid.Measure
east - westbefore normalization. If the raw span is at least360, set the bounds to[-180, 180]before the wrapped-bounds split. Add a test for an unwrapped full-world interval.🤖 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/plugins/src/plugins/maplibre-dggal.ts` around lines 471 - 497, Update dggalGridForBounds to measure the raw longitude span before normalizeLon modifies west and east. For spans of at least 360 degrees, set the normalized bounds to -180 and 180 before the east < west split, preserving full-world coverage. Add a test covering an unwrapped interval such as [0, south, 360, north].apps/geolibre-desktop/src/hooks/usePlugins.ts (1)
1336-1352: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMatch the client validation to the GitHub proxy contract.
githubRawVectorProxyUrl()accepts a GitHub raw URL with a query string. The Worker rejects that source URL because its/github-rawhandler requiresupstream.search === "". After native and direct browser fetches fail, the proxy path returns HTTP 400 for otherwise matched URLs.Reject URLs with
url.search !== ""here, or intentionally extend the Worker contract and its validation tests.🤖 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/usePlugins.ts` around lines 1336 - 1352, Update githubRawVectorProxyUrl() to reject GitHub raw URLs when url.search is non-empty, alongside its existing protocol, hostname, and pathname validation. Keep queryless URLs flowing to GITHUB_RAW_VECTOR_PROXY so the client matches the Worker’s /github-raw contract.
🤖 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/hooks/usePlugins.ts`:
- Around line 1336-1352: Update githubRawVectorProxyUrl() to reject GitHub raw
URLs when url.search is non-empty, alongside its existing protocol, hostname,
and pathname validation. Keep queryless URLs flowing to GITHUB_RAW_VECTOR_PROXY
so the client matches the Worker’s /github-raw contract.
In `@packages/plugins/src/plugins/maplibre-dggal.ts`:
- Around line 471-497: Update dggalGridForBounds to measure the raw longitude
span before normalizeLon modifies west and east. For spans of at least 360
degrees, set the normalized bounds to -180 and 180 before the east < west split,
preserving full-world coverage. Add a test covering an unwrapped interval such
as [0, south, 360, north].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5352afe-abea-4894-989b-ce10fe750cbb
📒 Files selected for processing (5)
apps/geolibre-desktop/src/hooks/usePlugins.tspackages/plugins/src/plugins/maplibre-dggal.tspackages/plugins/src/plugins/maplibre-dggrid.tstests/dggal-plugin.test.tstests/dggrid-plugin.test.ts
Measure longitude span before normalizing endpoints, and reject raw URLs with query strings so the client matches the Worker contract.
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/hooks/usePlugins.ts`:
- Around line 1343-1347: Update the URL validation in githubRawVectorProxyUrl to
reject credentials, non-default ports, and fragments before constructing the
public proxy URL; validate that username and password are empty, port is empty
or the HTTPS default, and hash is empty alongside the existing protocol,
hostname, search, and pathname checks.
In `@tests/dggal-plugin.test.ts`:
- Around line 140-142: Update the full-world test around dggalGridForBounds to
collect each returned feature’s zone ID and assert that the IDs are unique, then
retain the comparison between the unique/full-world result count and
dggrs.countZones(4). Ensure the assertions verify both no duplicates and
complete zone coverage.
🪄 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: 3aa6e093-8fac-4cb4-9e03-123c46935cb2
📒 Files selected for processing (3)
apps/geolibre-desktop/src/hooks/usePlugins.tspackages/plugins/src/plugins/maplibre-dggal.tstests/dggal-plugin.test.ts
… zones. Reject credentialed, non-default-port, and fragment URLs before proxying, and verify the full-world fill has no duplicate zone IDs.
giswqs
enabled auto-merge (squash)
August 3, 2026 20:51
Member
|
The dependency audit CI failed. That's the one last blocker. |
Member
|
The dependency audit failure is not caused by your PR. It is being fixed in #1688. You can rebase from main after the PR 1688 is merged. |
giswqs
dismissed
coderabbitai[bot]’s stale review
August 3, 2026 21:14
Dismissing to unblock merge: remaining thread is a trivial style nit (wrapLongitude simplification); will follow up separately.
Member
|
@thangqd This is an amazing plugin! I really like the automatic resolution feature. Thank you for your contribution. |
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 by CodeRabbit
New Features
Improvements
Tests