fix(arcgis): load feature services by viewport - #1765
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:
📝 WalkthroughWalkthroughArcGIS interactive feature layers now appear with empty GeoJSON data, then load features for the settled viewport. Paging supports incremental publication, cancellation, stale-result filtering, antimeridian splitting, restoration, bounded refresh, and cleanup. Headless callers retain complete downloads. ChangesArcGIS viewport loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Map
participant ArcGISPlugin
participant FeatureServer
participant GeoJSONLayer
Map->>ArcGISPlugin: create viewport layer
ArcGISPlugin->>GeoJSONLayer: create empty layer
ArcGISPlugin->>FeatureServer: request bounded feature pages
FeatureServer-->>ArcGISPlugin: return page features
ArcGISPlugin->>GeoJSONLayer: publish incremental features
Map->>ArcGISPlugin: moveend or reload
ArcGISPlugin->>FeatureServer: cancel prior request and query new bounds
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
|
![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 `@packages/plugins/src/plugins/arcgis-layer.ts`:
- Around line 453-487: In the load function, after defining publish and before
starting fetchArcGISFeaturePages, clear the layer by publishing an empty feature
collection through publish. Keep the existing request sequencing and fetch
behavior unchanged.
- Around line 459-470: Update the viewport query logic around the bbox and query
construction to detect antimeridian-crossing or west-greater-than-east longitude
bounds before clamping. Build separate ArcGIS envelope queries for the eastern
and western segments, execute both, then merge results using each feature’s ID
to remove duplicates; retain the existing single-query path for non-wrapped
bounds and add tests covering wrapped and west > east viewports.
In `@tests/arcgis-feature-layer.test.ts`:
- Around line 224-273: Extend the “adds an interactive layer immediately and
queries the current viewport” test to mutate map bounds, invoke the stored
moveend listener, and verify a second query uses the new viewport. Resolve the
initial request before resolving the replacement request, then assert only the
new response is published; remove the layer and assert the registered moveend
listener is passed to map.off.
🪄 Autofix
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: 0e1936ee-871e-4051-8fc3-ddc0d211fdcd
📒 Files selected for processing (2)
packages/plugins/src/plugins/arcgis-layer.tstests/arcgis-feature-layer.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Since Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
🔍 GitHub Pages PR preview
|
| pageSize: options.pageSize, | ||
| }, | ||
| metadata: { sourceKind: ARCGIS_FEATURE_SOURCE_KIND }, | ||
| metadata: { sourceKind: ARCGIS_FEATURE_SOURCE_KIND, viewportLoading: Boolean(map) }, |
There was a problem hiding this comment.
Quality/UX (high confidence): once viewportLoading is true, layer.geojson only ever holds the features intersecting the last-loaded viewport, never the complete dataset — but nothing downstream is aware of that:
- The Attribute Table (
AttributeTable.tsx) readslayer.geojson.features.lengthstraight into its "N features" status line, which will now read as a complete count when it's actually viewport-scoped. - Export (
vector-export.ts→resolveLayerGeojson, used byLayerPanel.tsx'shandleExportLayer/handleSaveEditsToSource) readslayer.geojsondirectly, so exporting or writing back a viewport-loading ArcGIS layer silently produces a file containing only whatever happened to be loaded for the last-viewed extent, with no warning that it's partial.
metadata.viewportLoading is set here but appears to have no other consumer in the codebase. Worth surfacing this to the user in at least the Attribute Table and Export flows (e.g. a "partial data — pan/zoom to load more" notice), since silently truncated exports are easy to miss.
There was a problem hiding this comment.
Agreed this is a real gap, but leaving it out of this PR — it is a UX change spanning the Attribute Table status line, the export/save-edits flows in LayerPanel, and a new string across 18 locale catalogs, none of which this PR otherwise touches. metadata.viewportLoading has a second consumer as of ea2c38a (refresh routes through the viewport loader instead of replaying the full download), so the flag is no longer dead. Leaving this thread open for a follow-up on surfacing partial data in the table and export.
|
All six inline comments posted successfully. Now finalizing the summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- Split an antimeridian-crossing viewport into two ArcGIS envelopes instead of clamping each edge on its own, which inverted the envelope (west 170, east -170) or collapsed it to zero width (west 200, east 210) and made the service answer with nothing for that part of the screen. Results from the two halves are merged and deduplicated by ObjectID. - Fold the per-layer store subscription into one shared, `??=`-guarded subscription (`ensureArcGISFeatureLoaderCleanup`), matching the existing `ensureArcGISStoreCleanup` pattern, so N viewport layers no longer mean N full-store subscribers each scanning `state.layers` on every update. - Route refresh for a viewport-loading layer through the loader's bounded query (`reloadArcGISViewportLayer`) rather than replaying the unbounded paged download, which would have pulled the whole service in on every refresh cycle. - Record a failed viewport query on `connection.lastError` so the Layers panel surfaces it, instead of only logging to the console; the write is skipped when the state is unchanged so a healthy pan does not dirty the project. - Document that `maxFeatures` caps each viewport query in the interactive app, not the layer as a whole. - Extend the viewport test to pan mid-flight, assert the superseded response is discarded and the listener is detached on removal, and add a test for the antimeridian split.
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/layer-refresh.ts`:
- Around line 576-585: Handle superseded viewport reloads in the layer refresh
path: in apps/geolibre-desktop/src/lib/layer-refresh.ts lines 576-585, catch
await viewport failures and return the layer’s current features for DOMException
errors named AbortError, while rethrowing other errors. In
packages/plugins/src/plugins/arcgis-layer.ts lines 453-526, document at line 470
that load() rejects when superseded, or instead resolve superseded calls with
the current collection so reloadArcGISViewportLayer does not reject during
panning.
In `@tests/arcgis-feature-layer.test.ts`:
- Around line 347-378: Add a test alongside the existing ArcGIS viewport tests
that makes the viewport query reject, then asserts the connection’s lastError
contains the rejection message. Resolve a subsequent viewport query and assert
connection.lastError is reset to null, using the existing layer setup and fetch
stubs.
🪄 Autofix
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: 050d163f-e228-4a14-a42f-cb662c823883
📒 Files selected for processing (4)
apps/geolibre-desktop/src/lib/layer-refresh.tspackages/plugins/src/index.tspackages/plugins/src/plugins/arcgis-layer.tstests/arcgis-feature-layer.test.ts
Code reviewBugs
Performance / Quality
Security
CLAUDE.md
The three findings above (all interrelated around restored-project viewport layers) are posted as inline comments with more detail and suggested directions. |
- Stop a superseded viewport query from rejecting: `load()` now resolves with the layer's current features when a `moveend` aborts it, so a refresh that overlaps a pan is no longer reported as a failed refresh (and, under an `onFailure` of "clear", no longer wipes a healthy layer). - Re-attach viewport loaders on project load via the new `restoreArcGISViewportLayers`, wired into DesktopShell's restore effect. `metadata.viewportLoading` round-trips through the saved project, but only the Add Data flow started a loader, so a reopened project's layer stayed frozen on the extent it was saved with and its refresh fell back to the unbounded download. - Drop the Add Data dialog's `onProgress` from the options the loader keeps, rather than holding an unmounted dialog's closure for the layer's lifetime. - Test the connection.lastError path: a rejected viewport query records the message, and a later successful one clears it.
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 `@packages/plugins/src/plugins/arcgis-layer.ts`:
- Around line 506-523: Update the parallel envelope-fetch logic around
fetchArcGISFeaturePages so queryOptions.maxFeatures is enforced across the
entire viewport rather than independently per antimeridian request. Coordinate a
shared remaining-feature budget (including onPage pagination) or divide the
limit between envelopes, ensuring the combined pages never exceed the configured
maximum; add an antimeridian test covering maxFeatures.
- Around line 525-529: Update the catch block in the ArcGIS layer request flow
to return currentArcGISLayerGeojson(layerId) for any stale request where
sequence !== requestSequence, regardless of error type; only rethrow errors from
the current request. Add a regression test covering a first request rejected
with a non-AbortError after a replacement viewport request begins.
- Around line 578-584: Update the layer restoration logic around the existing
`arcgisFeatureLoaders` check to skip only when the stored loader is already
bound to the current `map`; when it belongs to a previous Map instance, stop
that loader before creating and attaching its replacement so the moveend
listener and bounds use the current map.
🪄 Autofix
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: 7ecf151d-21e3-4c5b-82be-51f75e0622b5
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/layout/DesktopShell.tsxpackages/plugins/src/index.tspackages/plugins/src/plugins/arcgis-layer.tstests/arcgis-feature-layer.test.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
I also looked for a stale-loader risk if the MapLibre map instance were ever recreated while a viewport layer is active (since |
- Ignore every error from a superseded viewport query, not just AbortError: a request that failed just before its abort landed was still setting connection.lastError and failing a concurrent refresh. - Await both antimeridian halves with `allSettled` before reporting, so a surviving half can no longer publish pages over an already-reported error with nothing left to clear it. - Trim the merged split-viewport result to `maxFeatures`, which each envelope request was otherwise honoring on its own — a limit of N could leave the layer holding 2N. - Rebind a restored loader when the map instance changes instead of skipping on the mere presence of a loader, which left the listener on a dead map. - Refusing rather than falling through: a viewport-loading layer with no live loader yet (a just-reopened project still resolving service metadata) now reports that it is not ready, instead of silently replaying the unbounded download. - Tests for the stale non-abort failure and for the split-viewport cap.
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)
packages/plugins/src/plugins/arcgis-layer.ts (1)
615-625: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent an obsolete restore request from replacing the current map loader.
When the map changes while
fetchArcGISJsonis pending, both restore calls pass the layer-exists check. If the older request resolves last, Line 623 stops the loader bound to the current map and creates a loader on the obsolete map. Latermoveendevents and reloads then use stale bounds.Before starting the loader, require
app.getMap?.() === map. Apply the same guard in the rejection handler so an obsolete request cannot setconnection.lastErroron the current layer. Add a deferred-metadata test that resolves the newer map request before the older one.Proposed fix
.then((layerInfo) => { - if (!useAppStore.getState().layers.some((entry) => entry.id === layerId)) return; + if ( + app.getMap?.() !== map || + !useAppStore.getState().layers.some((entry) => entry.id === layerId) + ) { + return; + } startArcGISViewportLoader(layerId, map, queryUrl, options, layerInfo); }) - .catch((error: unknown) => handleArcGISViewportError(layerId, error)); + .catch((error: unknown) => { + if (app.getMap?.() === map) handleArcGISViewportError(layerId, error); + });🤖 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/arcgis-layer.ts` around lines 615 - 625, Update the fetchArcGISJson completion and rejection handlers around startArcGISViewportLoader to require both the layer’s continued existence and useAppStore.getState().app.getMap?.() === map before proceeding. Apply the same map guard before handleArcGISViewportError so obsolete requests cannot affect the current layer, and add a deferred-metadata test that resolves the newer map request before the older one.
🤖 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 `@packages/plugins/src/plugins/arcgis-layer.ts`:
- Around line 615-625: Update the fetchArcGISJson completion and rejection
handlers around startArcGISViewportLoader to require both the layer’s continued
existence and useAppStore.getState().app.getMap?.() === map before proceeding.
Apply the same map guard before handleArcGISViewportError so obsolete requests
cannot affect the current layer, and add a deferred-metadata test that resolves
the newer map request before the older one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d28fd79-f0f9-4e0e-a160-b455511cbc2f
📒 Files selected for processing (3)
apps/geolibre-desktop/src/lib/layer-refresh.tspackages/plugins/src/plugins/arcgis-layer.tstests/arcgis-feature-layer.test.ts
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the antimeridian-splitting, request-supersession, and error-reporting logic is carefully reasoned through and the accompanying tests cover the tricky race conditions well; the concerns above are edge cases in the project-restore path rather than issues with the core viewport-loading feature. |
- Register a restored loader synchronously and fetch the service metadata lazily on its first query, memoized with the memo cleared on failure. A metadata fetch that failed once left the layer permanently unbound, with refresh reporting a transient-sounding error that nothing ever retried; now the next pan or refresh retries it, and there is no window in which a refresh finds the layer unbound at all. - Reword the refresh error accordingly: no loader now means no map, not a race with startup. - Drop the unreachable abort branch in fetchArcGISFeatureCount — a viewport query is the only cancellable caller and planArcGISPaging skips the count for those, so the signal was always undefined where it ran. - Test restoreArcGISViewportLayers end to end: synchronous binding, a failed metadata read reported and retried on the next pan, rebinding when the map instance changes, and reloadArcGISViewportLayer re-querying the viewport.
|
Both inline comments posted successfully. Code reviewThis PR adds viewport-bound loading for ArcGIS FeatureServer layers: features load progressively for the current map extent, stale requests are cancelled on pan/zoom, restored projects re-bind their loaders, and headless/API consumers keep the full paged download. The implementation is unusually well-documented and defensively coded (sequence numbers to guard against out-of-order responses, Bugs: None found. Security: None found. URL construction goes through Performance:
Quality: The new CLAUDE.md: No violations found. No new external tile/map hosts, no hand-edited generated catalogs, and the new user-facing error string ("This layer is not bound to a map viewport...") follows the same untranslated raw- |
Reported against Vicmap_Parcel: the layer was added but never showed any features, with only a console error to say why. The service answers a geometry-bearing query over a metro-scale envelope in 20-35s when healthy, and exceeds its own ~56s timeout when busy. It reports that timeout as HTTP 200 carrying an error envelope whose code is 400 and whose detail blames the query parameters — which are correct; the identical request succeeds on a retry. The viewport loader treated it as a hard failure, so the layer stayed empty. - Throw a typed ArcGISQueryError carrying the HTTP status and the envelope code, so a timeout is distinguishable from an auth or network fault. - Retry a transient failure once per envelope walk, skipped when a pan has already aborted the request. - Report guidance the user can act on when the retry fails too, instead of ArcGIS's own wording, which sends them to check parameters that are fine. - Clear connection.lastError on the first published page rather than at the end of the walk: a dense extent pages for tens of seconds, and a stale error sitting on screen while features stream in reads as a live failure. Verified against the live service, not only the mocked tests: the metro extent loads (2000 features, no error), a suburb extent loads (6000), and no stale error survives either. The timeout path is covered by regression tests built from the real error envelope, both confirmed to fail without the fix.
| function startArcGISViewportLoader( | ||
| layerId: string, | ||
| map: maplibregl.Map, | ||
| queryUrl: string, | ||
| options: ArcGISLayerOptions, | ||
| resolveLayerInfo: () => Promise<ArcGISFeatureLayerInfo>, | ||
| ): void { | ||
| let abort: AbortController | null = null; | ||
| let requestSequence = 0; | ||
| // The Add Data dialog's progress callback belongs to the initial download. It | ||
| // is unmounted well before the first viewport query lands, so carrying it for | ||
| // the layer's lifetime would keep that closure alive to no purpose. | ||
| const queryOptions: ArcGISLayerOptions = { ...options, onProgress: undefined }; | ||
| const maxFeatures = positiveInteger(options.maxFeatures); | ||
| const loader: ArcGISViewportLoader = { | ||
| abort: null, | ||
| load: () => Promise.resolve({ type: "FeatureCollection", features: [] }), | ||
| map, | ||
| move: () => undefined, | ||
| }; | ||
|
|
||
| /** | ||
| * Query the map's current extent, publishing each page as it lands. | ||
| * | ||
| * Resolves with whatever the layer holds — never rejects, whatever the | ||
| * error — when a newer viewport supersedes the call: the replacement is | ||
| * already publishing, so a failure the superseded request happened to hit | ||
| * (an abort, or a request that failed just before its abort landed) is not | ||
| * this layer's problem. A refresh running concurrently with a pan must not be | ||
| * reported as a failed refresh (and, under an `onFailure` of `"clear"`, wipe | ||
| * a layer that is perfectly healthy). | ||
| */ | ||
| const load = async (): Promise<FeatureCollection> => { | ||
| abort?.abort(); | ||
| const controller = new AbortController(); | ||
| abort = controller; | ||
| loader.abort = controller; | ||
| const sequence = ++requestSequence; | ||
| let layerInfo: ArcGISFeatureLayerInfo; | ||
| try { | ||
| layerInfo = await resolveLayerInfo(); | ||
| } catch (error) { | ||
| if (sequence !== requestSequence) return currentArcGISLayerGeojson(layerId); | ||
| throw error; | ||
| } | ||
| const envelopes = arcgisViewportEnvelopes(map.getBounds()); | ||
| // One bucket per envelope, so a viewport split across the antimeridian | ||
| // publishes both halves together instead of each replacing the other. | ||
| const pages: Feature[][] = envelopes.map(() => []); | ||
| const collect = (): FeatureCollection => { | ||
| const features = mergeArcGISViewportFeatures(pages, layerInfo.objectIdField); | ||
| // A split viewport issues one request per envelope, each honoring | ||
| // `maxFeatures` on its own, so the merge is trimmed to keep the option a | ||
| // bound on what the layer holds for one viewport rather than per half. | ||
| return { | ||
| type: "FeatureCollection", | ||
| features: maxFeatures === null ? features : features.slice(0, maxFeatures), |
There was a problem hiding this comment.
Performance: viewport queries have no ceiling when maxFeatures is unset (the default), and arcgisViewportEnvelopes falls back to the full [-180, south, 180, north] world envelope whenever the viewport spans ≥360° of longitude (e.g. the user zooms out past the whole world, which is easy to do after fitBounds if the layer's own extent is large). In that case remainingArcGISFeatures never caps the walk (plan.maxFeatures === null ⇒ wanted = pageSize), so the "bounded" viewport query degenerates into essentially the same unbounded where=1=1 download that this PR (and #1745) exists to avoid — and it re-runs on every subsequent moveend while the viewport stays that wide, not just once.
Since the Add Data dialog leaves maxFeatures optional with no default (ArcGISSource.tsx), this is reachable without any unusual configuration, just panning/zooming out on a layer with a large extent. Consider clamping viewport queries to a page or two when no maxFeatures is set and the envelope is very wide (or gating the query on a minimum zoom level), rather than letting MAX_ARCGIS_PAGES (5000 pages) be the only backstop.
Confidence: medium — the mechanism is real, but I can't verify how often users actually reach a whole-world/near-whole-world viewport on one of these layers in practice.
| ): Promise<number | null> { | ||
| try { | ||
| const response = await fetch( | ||
| appendArcGISParams(queryUrl, { |
There was a problem hiding this comment.
Nit: fetchArcGISFeatureCount now receives the full paging params (including outFields: "*" and returnGeometry: "true") instead of just the token, and spreads them into the returnCountOnly=true request. ArcGIS should ignore these for a count-only query, so this is likely harmless, but it's dead weight on every count request and slightly obscures that only the token (and, for split-viewport calls, the geometry filter) actually matter here. Worth a { token: params.token, geometry: params.geometry, ... } pick instead of spreading everything, if only for clarity.
Confidence: low — cosmetic, not a functional issue as far as I can tell.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Posted two inline comments: one on the unbounded-viewport-query performance concern, one a minor nit on the count-request params. |
Summary
Verification
node --import tsx --test tests/arcgis-feature-layer.test.tsnpm run buildFixes #1756
Summary by CodeRabbit
New Features
Bug Fixes