fix(arcgis): load feature layers as GeoJSON so labels can be styled#868
Conversation
ArcGIS feature layers added from the Add Data sample dropdown were registered as external-native layers whose GeoJSON source was a remote `/query?f=geojson` URL. Their features were never stored on the layer, so the Style panel's label field showed "No attributes found" and the label sync path is skipped entirely for external-native layers. That left the label formatting options (uppercase, offset, rotation) inapplicable to these suggested vector layers. Feature layers are just attributed vector data, so fetch the query result up front and hand it to the store's GeoJSON layer path instead. They now render as first-class vector layers with their attributes available, which unlocks labels (and their formatting), the attribute table, identify, symbology, and export. Vector tile layers keep the external-native runtime path. Fixes #867
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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 (3)
📝 WalkthroughWalkthroughArcGIS feature layers now load through a GeoJSON query path with metadata lookup, response validation, store insertion, and optional extent-based bounds fitting. Inline GeoJSON source creation now preserves source attribution. ChangesArcGIS feature-layer GeoJSON path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Pages preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Quality
Security / Performance / CLAUDE.mdNothing else worth raising. The token-in-URL pattern is pre-existing across all What was checked
|
- fetchArcGISGeoJson: warn when an ArcGIS query is truncated at the service's maxRecordCount (exceededTransferLimit). The partial dataset still loads — throwing would make large feature layers unloadable, a regression from the prior URL-source path that rendered the same subset. - Add tests for the exceededTransferLimit warning path and the portal-item resolution path (portal item URL -> service URL -> query).
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/arcgis-feature-layer.test.ts`:
- Around line 148-156: Strengthen the ArcGIS feature layer test by verifying the
actual fetch flow instead of returning the same fallback response for all
non-query URLs. In the fetch mock inside arcgis-feature-layer.test.ts, capture
requested URLs and assert that the portal-item resolver path hits
/content/items/... and that the resolved service request hits the service /query
URL; use the existing test setup around globalThis.fetch and the
portal-item/serviceUrl assertions to make the test fail if either request is
skipped.
🪄 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: ae11d4c0-4937-4f98-9c33-d49ac0f7d2aa
📒 Files selected for processing (2)
packages/plugins/src/plugins/arcgis-layer.tstests/arcgis-feature-layer.test.ts
- Strengthen the portal-item test: capture fetch URLs and assert both the portal /content/items/<id> lookup and a query against the resolved service URL actually occurred, so the test cannot pass if the resolver skips the portal path.
Code reviewThis PR replaces the opaque external-native fallback path for ArcGIS feature layers with a clean GeoJSON fetch + `addGeoJsonLayer` store call, which correctly unlocks labels, symbology, and the attribute table. The approach is sound and the tests cover the main flows well. Three issues need attention before merge. Bugs1. New ArcGIS GeoJSON layers are incorrectly treated as "refreshable" (high confidence) `addGeoJsonLayer` always produces `metadata: {}` — no `sourceKind`, no `externalNativeLayer`. `refreshSourceUrl` in `layer-refresh.ts` returns the raw `FeatureServer/0` URL as the refresh endpoint because the `sourceKind` guard short-circuits on `undefined` and the `externalNativeLayer` guard is absent. When the user triggers a refresh (or a scheduled auto-refresh runs), `fetchGeoJsonFeatureCollection` is called with the service-metadata URL and throws `"The response is not a GeoJSON FeatureCollection."` The Refresh button should not appear for these layers at all. Fix: call `useAppStore.getState().updateLayer(id, { metadata: { sourceKind: "arcgis-feature" } })` immediately after `addGeoJsonLayer`. (Inline comment on line 263.) 2. `response.json()` in `fetchArcGISGeoJson` throws an uninformative `SyntaxError` for 200 + HTML responses (high confidence) ArcGIS Enterprise and many WAFs return HTTP 200 with an HTML login page when a token is missing or expired. `response.ok` is `true` so the HTTP-error guard does not fire, then `response.json()` throws `SyntaxError: Unexpected token '<'` rather than the helpful message one line below. The pattern used by `fetchGeoJsonFeatureCollection` in `layer-refresh.ts` — read as text, sniff for `<`, then parse — should be applied here too. (Inline comment on line 287.) Performance3. Attribution (`copyrightText`) is silently dropped (medium confidence) The old code set `attribution: layerInfo.copyrightText || "ArcGIS Feature Service"` on the MapLibre source spec. `addGeoJsonLayer` constructs `source: { type: "geojson" }` with no attribution, and `syncGeoJsonLayer` does not inject one. The copyright watermark disappears from MapLibre's `AttributionControl` for any feature service that declares `copyrightText`. (Inline comment on line 255.) Quality4. Dead branch in `createArcGISStoreLayer` (low confidence — nit) `const sourceType = options.layerType === "feature" ? "geojson" : "vector";` (line 531) — feature layers now exit at line 90–92 and never reach this function, so the `"feature"` arm is unreachable. The ternary should be simplified to `const sourceType = "vector";` to avoid misleading future readers. This line is unchanged in the diff so it could not receive an inline comment, but it is a direct consequence of this PR. |
| if (json.exceededTransferLimit) { | ||
| console.warn( | ||
| `[GeoLibre] ArcGIS feature query was truncated at the service record ` + | ||
| `limit; loaded ${json.features.length} features (partial dataset).`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Silent truncation is a meaningful UX regression
console.warn is the only signal when exceededTransferLimit is true, so users who add a service with more features than its maxRecordCount (often 1 000–2 000) silently get a partial layer with no in-product indication. In the old URL-backed GeoJSON source the same truncation happened, but the layer was dynamic (MapLibre re-fetched on demand). Now the truncated copy is permanently frozen in the Zustand store and appears complete in the attribute table and any export.
At minimum, either:
- Surface a non-blocking toast/notification in the UI at the plugin level (if the
appAPI exposes one), or - Throw a typed warning object that the Add Data dialog's error boundary can render distinctly from a hard error, so the layer still loads but the user sees "Loaded N features — service limit reached."
A comment on line 302 claims this is "the same subset the previous URL-source path rendered," but that framing misses the key difference: the subset is now static and stored.
There was a problem hiding this comment.
Partially pushing back on the framing here. A MapLibre geojson source loads its data once and does not re-fetch on demand, so the previous URL-source path was equally static and equally capped at maxRecordCount — there is no loss of dynamism, and this change actually adds a truncation signal where there was none. A user-facing toast would be a nice improvement, but the plugin app API exposes no notification/toast hook, so surfacing one would require a broader API addition that is out of scope for this fix. Leaving this thread open for your call on whether to add that API separately; happy to do it in a follow-up if you'd like.
- Refresh: persist the GeoJSON `/query` endpoint (not the FeatureServer base URL, which returns the service-description envelope) as the layer's sourcePath, so a GeoJSON refresh re-fetches valid features. The access token is kept out of the persisted URL and only appended to the one-off load request, so it is never written to a saved project. - Robustness: read the query response as text and reject a leading-`<` HTML body (a 200 login/redirect page from ArcGIS Enterprise or a WAF) with a clear message instead of a raw JSON.parse SyntaxError. - Attribution: carry the service's copyrightText onto the layer's source.attribution and propagate source.attribution into the MapLibre GeoJSON source in syncGeoJsonLayer, restoring the copyright watermark. - Tests: cover the refresh-URL/token-not-persisted, HTML-200, and attribution behaviors.
Code reviewThis PR replaces the old URL-backed GeoJSON source for ArcGIS feature layers with an upfront fetch, making features first-class GeoJSON layers in the store. That correctly enables labels, the attribute table, and symbology for feature layers. The overall approach is sound and the test coverage is solid. Bugs[HIGH] URL-sourced feature layers are incorrectly treated as refreshable ( [MEDIUM] Portal-item Performance / Correctness[MEDIUM] Silent truncation when Tests[LOW] Fragile mock discriminator ( Checked and found OK
|
Code reviewReviewed Bugs
Quality
Tests
CLAUDE.mdNo violations found. The new code does not mutate MapLibre directly from UI (correct store-driven pattern), does not add hardcoded tile/host URLs that would need CSP additions, and the test file follows the existing |
1 participant
Problem
Fixes #867. Label formatting options (uppercase, offset, rotation, and labeling in general) were not applicable to ArcGIS vector layers added from the Add Data Load sample data dropdown (e.g. "US major cities (feature)").
Root cause
ArcGIS feature layers were registered as external-native layers (
externalNativeLayer: true) whose GeoJSON source was a remote/query?f=geojsonURL. The features were never stored on the layer record, so:getAttributePropertyNames()in the Style panel found no attributes → the Label field dropdown was disabled showing "No attributes found", so no field could be picked.syncLayer()returns early for external-native layers (packages/map/src/layer-sync.ts), so the label symbol layer that carriestext-transform/text-offset/text-rotatewas never created.Inline-GeoJSON suggested layers (WFS, Delimited Text, GPX) store
layer.geojsonand were unaffected.Fix
A feature layer is just attributed vector data, so fetch the
/query?f=geojsonresult up front and hand it to the store's GeoJSON layer path instead of building an external-native runtime layer. Feature layers now render as first-classtype: "geojson"vector layers with their attributes available, which unlocks labels (and their uppercase/offset/rotation formatting), the attribute table, identify, symbology, and export. Vector tile layers keep the external-native runtime path. The now-dead fallback helpers were removed.Verification
Reproduced and verified in the real app with Playwright using the ArcGIS "US major cities (feature)" sample:
arcgisand the Label field dropdown was disabled ("No attributes found").vector, the Label field lists the real attributes (NAME, POPULATION, STATE_ABBR, …), labels render, and UPPERCASE + 90° rotation visibly apply. Confirmed in both light and dark themes.Added
tests/arcgis-feature-layer.test.tscovering the GeoJSON conversion (attributes preserved, bounds fitted) and the non-GeoJSON error path. Full frontend suite (1608 tests),npm run build, and pre-commit all pass.Summary by CodeRabbit
/query?f=geojsonresponses, including clear errors for non-GeoJSON (e.g., HTML/token issues).