Feat/shapefile map layer source - #176
Merged
Merged
Conversation
Adds a projection table for CRSes OpenLayers cannot resolve on its own, registered at module evaluation so it is in place before layers construct. Layers are built concurrently, so a registration that waited on anything async would race them. Registration is scoped deliberately. `register` builds pairwise transforms across every registered code, so its cost is quadratic -- 99ms for two definitions on top of proj4's built-ins, and hundreds of milliseconds at a State-Plane-sized set. This module is a static import, so an unbounded init set would be a first-render regression on every dashboard. Only codes a layer actually names are registered up front; `ensureProjection` handles the rest on demand. Definitions, extents and control points come from PROJ's EPSG database rather than being hand-derived, and the control points sit away from each projection's origin so they exercise the standard parallels and scale factor. proj4 agrees with PROJ on both to sub-millimetre, which makes the round-trip test a cross-implementation check rather than a self-consistency one. Three behaviors worth naming, each found by measurement: - `register` cannot supply an extent, so extents are applied afterward. - A definition is validated before being registered, not after: `register` constructs a transform for every pair of registered codes, so one unusable definition makes it throw and takes working projections down with it. The probe uses the definition's own centre, since a fixed point is outside many projections' domains. - proj4 implements the ESRI spelling of Albers but not the OGC one, which fails silently with non-finite coordinates. Now reported rather than rendering features nowhere. A layer's own WKT never overwrites a definition that already resolves. The registry is global to the browser session, so letting one layer's parameters replace a code others resolve through would make rendering depend on which dashboard was opened first. Unresolvable WKT registers under a synthetic code, never under a claimed authority code. The raster auto-fit will not adopt a registered-but-not-native projection as the view projection. Adoption calls setView and publishes the adopted code into the map-extent variable other visualizations read; widening it is a separate change with its own verification. Such a raster still renders, by reprojection. Dependencies for the whole feature land here rather than across three commits: proj4, shapefile, fflate, @mapbox/geojson-rewind, wkt-parser. All exact-pinned; fflate matches the version already resolved transitively to avoid a duplicate install. wkt-parser is declared directly because proj4 does not re-export its parser and the outermost AUTHORITY node is needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ling A map-free acquisition step: URL validation, sibling derivation, fetching, decompression and a bounded component cache. It returns raw buffers and knows nothing about what a shapefile means, so the parser choice -- which carries the real risk here -- can change without disturbing any of it. The size ceiling binds on each member's *declared* size, read from the local header before any data flows, and refuses by never starting that member. Summing bytes as they arrive does not work: an 8 MiB expansion arrives in a single callback, so a running total only notices once the payload is already allocated and inflated, which is the cost the ceiling exists to prevent. A member declaring no size falls back to counting. Only shapefile components are ever started, so a bomb parked in an unrelated member costs nothing -- covered by a test. fflate's Unzip carries only a pass-through decoder, so without registering the inflate decoder every member of a real archive throws on start. That is a total failure rather than a degradation, and it now has a regression test. Bodies are read whole rather than streamed. Aborting rejects the read and terminates the transfer, which is what cancellation needs, and the configured test environment exposes no response stream at all -- so a stream-reader implementation could not have been exercised. Absence and failure stay distinguishable on the sibling path: a 404 on an optional component means absent, any other status is reported. A transient 403 routed into the absent path would fall back to the author-supplied projection and draw features somewhere else entirely, with no error. Two failure modes get accurate messages rather than misleading ones. A response is checked for markup, and the buffer for the zip magic number, because a portal returning an HTML error page with a 200 would otherwise be reported as an archive containing no .shp entry. And a fetch-stage failure names cross-origin policy, an unreachable host, a missing file and an expired signature together, since a browser cannot tell them apart. The cache holds component buffers keyed on resolved URL, not parsed features: buffers are already under the ceiling by construction so a small entry count has an exact memory bound, while parsed GeoJSON runs several times the archive size. A hit skips the network hop and the decompression and still re-parses. Failures are never cached, so a retry retries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion Parses the geometry, resolves the coordinate reference, normalizes ring winding, and returns a plain GeoJSON feature collection whose `crs` names its projection -- the same payload shape the existing vector-swap path already consumes, so the two vector paths stay interchangeable and no OpenLayers object is built here. The parser choice is now settled rather than assumed. Fixtures are real shapefile bytes generated by pyshp, and the expectations come from pyshp reading them back through its own independent ring-nesting implementation -- so the fidelity test is a cross-implementation check. It passes: a polygon with an interior ring reads as one polygon with a hole, and a multi-part record as one MultiPolygon, both matching pyshp coordinate for coordinate. That was the largest risk in the plan. The browser build is imported by name. The package resolves to a Node build under the test runner and a browser build under the bundler, and a fidelity guarantee measured against an artifact that never ships is worth nothing. That needs TextDecoder, which jsdom lacks, so setupTests now supplies it -- which also moves tests closer to a real browser than the Node build's bundled decoder. Absence and failure stay distinct. A genuinely missing .prj falls back to the author-supplied projection; a .prj that is present but unresolvable is reported, never quietly replaced by the fallback, because the file said what it was and drawing it with a guessed projection would put the features somewhere else with no error. One test-harness trap worth recording: with a native TextEncoder available, fflate's strToU8 returns a Uint8Array from the Node realm, so fflate's own instanceof check fails and zipSync recurses into the byte indices, producing archive members named "basins.shp/0/" instead of a file. Fixtures now build their bytes in the test realm. Only fixture construction was affected -- nothing in the application builds archives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vocabulary Extracts `readFeatureCollection` from `swapVectorLayerFeatures` so the shapefile source's loader can read a collection the same way, and adds one module for the names the two async vector paths share. The loader cannot call the swap function itself: that clears every feature already on the source, whereas a loader is additive inside its success callback. Worth recording alongside it -- clearing a source does *not* reset OpenLayers' loaded-extent bookkeeping, so it is not a usable retry primitive. `refresh()` is the one that works. Taking the projection as a per-call argument rather than baking it in when the source is built is what lets a loader read features against the view as it stands at insertion, instead of as it stood when its fetch began. Covered by a test that reads the same collection into two projections. The status vocabulary carries one judgment: retry is offered only for fetch-stage failures. A missing projection, an unresolvable coordinate system, a malformed component and a source over the size ceiling all need the author to change something, so a retry button for them invites a viewer to re-download megabytes and fail identically. The plugin path already gates its own retry by failure kind for the same reason. The two paths stay separate by design -- one pushes on its own schedule, the other is pulled by OpenLayers when a layer renders -- so this shares their names rather than their lifecycle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registers the type end to end: the authoring registry, the module mapping, both dispatch branches in moduleLoader, and an explicit layer-type guard. Features load through OpenLayers' own loader hook rather than being fetched ahead of construction. That is what makes the projection requirement expressible at all -- moduleLoader receives a projection string captured at task start, but the loader is handed the live one when it runs, and its success/failure callbacks drive the load-event triple. It also means the loader is not called until the layer is actually mounted and rendering. The projection is read again at the moment features are inserted, not when the load began. A shapefile is the slowest-loading vector source in the app, so it is the one most exposed to a sibling raster's auto-fit changing the view mid-load -- features parsed into the outgoing projection are drawn thousands of kilometres off screen while still reporting the right feature count. Covered by a test that deliberately makes the construction-time and invocation-time projections both wrong. The dispatch for client-loading types is duplicated across the module-cache path and the post-import path, so both branches are wired and a test builds two shapefile sources to exercise each. One documented controller object on the source carries abort, status, error and reset, rather than three loose properties two modules discover by reaching into each other. Reset goes through `refresh()`, and the test asserts the loader ran a second time -- clearing the loaded extent alone leaves it un-invoked, which is how a retry button ends up doing nothing while its test passes. No layerId is assigned. Status lives on the source object, so a torn-down layer takes its status with it and a rebuilt one starts idle -- there is no external keyspace to invalidate, and none of the reused-identity hazards apply. The projection field takes a WKT or proj4 definition as well as a code, since a .prj-less shapefile in a CRS the table does not cover has no other authorable path. Handling that turned up a real gap: wkt-parser cannot read a proj4 string, so the definition is only parsed as WKT when it is shaped like WKT, and the probe reads the definition back from proj4 instead. `registerProjectionFromWkt` is renamed `registerProjectionDefinition` to match what it now accepts. Also fixes a latent flake of my own making: acquire.test.js assigned global.fetch without restoring it, and Jest reuses a worker across files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sites were re-derived by searching for the existing type-name literals rather than taken from a list, because that is the only way to find them: the capability is encoded as strings scattered across modules, and the failure mode of missing one is silent. Five needed changing; a sixth comes free. - The client-vector list gates both click queries and snapping. A type absent from it does not error -- the snap path falls through to the feature-service query and returns nothing. - Attribute discovery gains a branch that reads field names from the .dbf. - Style-field discovery routes through that same branch rather than getting a second implementation. The two are otherwise independent trees with different logic, and registering in only one gives working fields in one pane and an empty list in the other -- so a test asserts they agree. - The Style pane's supported-type list is a hard gate: absent from it, the tab renders a dead-end panel and styling the layer is impossible no matter what discovery returned. An existing test hardcoded that list in its expected message and has been updated. - The layer-property help text for clickTolerance and snapToFeatures enumerates eligible types and is user-visible. Snapping needs nothing further: it reads the shared client-vector list, and features arriving through a loading strategy become snappable as they load. The service-legend branch is deliberately untouched -- a shapefile layer takes the style-derived legend path, as a rule-styled vector should. Discovery reads the source, which acquisition caches against the resolved URL, so the style pane and the attributes pane reading in turn cost one fetch between them. When the source cannot be read, discovery returns an empty field list rather than throwing, so a failure surfaces through the layer's error path instead of breaking the editor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reconciliation sweep rebuilds every vector layer on any change to the layer array, so without this an opacity edit on an unrelated layer -- or one frame of a raster time-slider -- costs a full refetch, decompress and reparse of the whole archive. Preservation keys on the layer's name plus its resolved source URL. The keep predicate gains a branch rather than being generalized. The plugin-provenance check every existing plugin layer depends on is left exactly as it was, so preservation for those is untouched by this change. Preservation has a cost the add path was hiding: style is applied only when a layer is constructed, and the cosmetic prop sync carries only the props OpenLayers has first-class setters for. A preserved layer would therefore ignore a style-rule edit entirely -- which would contradict styling working on a shapefile layer at all. The style application is now factored out of the add path and re-applied to preserved layers when it differs from what was last applied. Two duplicate-layer hazards are closed. A run that has been superseded no longer adds its layer: it sits in no newer run's removal snapshot, so it would never be collected, leaving features drawn twice and every clicked feature reported twice in the popup. And the removal sweep now runs whenever the map actually holds layers rather than only when reconciliation state was recorded, because a run starting while a previous one is still loading sees no recorded state -- and gating removal on it let both runs' layers sit on the map. With no recorded state nothing is kept, so that case rebuilds rather than duplicates. In-flight loads are aborted when the layer is removed, when a run is superseded, and on unmount, so a fetch and decompression do not keep running for a layer nobody will see. The tests drive the loader directly, because OpenLayers pulls it only when a layer renders and a jsdom map has no size. That makes the assertion the right one anyway: a preserved layer keeps its source and its loaded-extent bookkeeping, so driving it again is a no-op, while a rebuilt layer loads from scratch. They wait on observable post-conditions rather than fixed delays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Failures and an in-flight indication go to the existing map-level alert, which is not gated on the author's layers-control toggle. That control is opt-in per dashboard and collapsed to an icon by default, so routing status only there would leave a viewer with nothing at all on any dashboard whose author disabled it -- and a failure rendering as a blank layer is the one outcome this must avoid. The control still carries the richer per-layer detail when enabled. Every failure class reaches the user with its own message: the observed and permitted size when a source is refused, the coordinate system when one cannot be resolved, the component and its status when one fails, and for a fetch-stage failure the candidate causes named together, since a browser cannot tell them apart. Retry is offered only where re-running the same request could succeed. A missing projection, an unresolvable coordinate system, a malformed component and a source over the size ceiling all need the author to change something, so a button for them would invite a viewer to re-download megabytes and fail identically. The plugin path already gates its own retry this way. Status is read from the source's controller rather than from a request id -- there is no backend request behind a client-parsed source, so the existing progress channel has nothing to report for one. It is mirrored into component state only so it can be rendered, and pruned to the layers actually on the map after each reconciliation: a rebuilt layer must not inherit the previous instance's failure, and a stale error must not suppress the replacement's loading indication. Retry goes through the source's refresh, which is the only primitive that causes the loader to run again, and a test drives the loader afterward to prove it did rather than asserting on internal state that would pass either way. R19 and R21 -- the author-facing remedy text and the elapsed-time escalation -- move to the editor unit, where the load action they attach to is built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… references Field discovery for a shapefile is triggered by the author rather than run automatically. The style pane's own discovery effect re-runs whenever its source props change, which for a typed URL means once per keystroke -- and each run here is a multi-megabyte download. The editor already established this pattern for remote GeoJSON with an explicit load action. One read serves both panes. Results are memoized against the resolved URL, and the style and attributes panes read from that instead of each fetching. The attributes pane's automatic read is suppressed for this source type the same way it already is for remote GeoJSON. Variable-input templates are resolved before fetching. The editor holds the raw configuration, so a templated URL would otherwise be requested literally -- guaranteed to fail, for exactly the sources variable inputs are most useful for. Storing no schema keeps the field list true to the source, but it moves staleness into the rules that name those fields: an upstream rename leaves them matching nothing while the layer still renders, so nothing fails and nobody is told. Discovery now compares what it found against every field the saved configuration references -- style rules including nested conditions, popup configuration, and attribute variables -- and names the ones that have gone missing. Field references are collected by walking the configuration rather than by known path, so a reference that moves does not silently stop being checked. The drift list renders in the Source tab, beside the action that produced it, rather than split across the panes that hold the settings. A style-rule reference that had gone missing would otherwise be invisible to an author who never opens the Attributes tab. Two things carried over from the surfacing unit, because they attach to this action: a fetch-stage failure names converting to GeoJSON as the supported alternative -- upload is not offered and a proxy is out of scope, so without it the author is told the cause and left with no move -- and a pending read past a fixed threshold escalates its message, since an indicator that never changes reads as a hang and invites paying for the read twice. A failure also states that saved style, popup and attribute settings are untouched. Discovery is held by the modal, which already hoists every pane's state, so no new context was needed to share it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…variables Three pre-existing defects in shared code, corrected together because a shapefile exposes all three and none is specific to it. Each changes behavior for every styled vector layer, so present-value behavior is covered by regression tests alongside. A field the feature does not carry can no longer satisfy a comparison. The negated operators were inverting into a match -- `!=` became `undefined !== x`, `notIn` became "not in the list", both true -- so one saved rule repainted every feature of a layer whose .dbf was missing or whose schema drifted upstream. The layer still rendered, so nothing failed and nobody was told. The presence checks run first and are untouched: asking whether an absent field is null has a real answer, and a rule styling "no data" depends on it. An empty string stays a present value. An attribute variable whose bound field is absent is now cleared rather than left alone. Skipping the write left the variable holding the previously clicked feature's value, so every dependent visualization kept rendering the wrong feature's data with no indication anything was stale -- the only path here that propagates a wrong value off the map. The same expression had a second defect: it tested truthiness, so a real 0, "" or false was indistinguishable from an absent field and dropped. A gage reading of zero left the previous gage's number on screen. Presence is now tested instead, and the field-then-alias fallback no longer discards a falsy field value either. The reprojection sweep runs on the other view-replacement path. It had one call site, on the raster auto-fit; the map-extent path replaces the view too, and the auto-fit adopts a projection without updating the state that view is rebuilt from -- so a later extent change reverted the projection underneath features that had already been moved once, leaving them drawn far off screen while still reporting the right feature count. Verified by reverting the fix and confirming the test fails. One existing test encoded the old variable-input behavior: it swiped to a feature whose value was the "Null" sentinel and asserted the variable kept the previous feature's value. That is the defect, so the test was updated rather than the fix weakened. Treating that sentinel as absent is a slight broadening of the requirement, which says "absent" -- the harm is identical however absence is spelled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a Shapefile section to the source tab alongside the other twelve, and updates the type lists that enumerate which sources support click tolerance, snapping and custom styling. Covers what an author has to decide: the two accepted URL forms and that one field takes either, that the coordinate system comes from the file's own .prj with the projection property as a fallback accepting a definition as well as a code, and that a missing .dbf still draws geometry but offers no fields. Three things are documented because they are surprising rather than because they are configurable. Reading the fields is an explicit action, since it is a large download and one read serves both tabs. Saved rules naming a field the source no longer has are listed rather than silently matching nothing. And the size limit applies to the decompressed components, so the number an author sees is not the size of the file they linked. The cross-origin note names the constraint concretely. Most agency portals send permissive headers, but Census TIGER -- among the most-used boundary sources in the country -- cannot be read from a browser at all, so the note says which way the common cases fall and what to do instead rather than describing CORS in the abstract. Verified against a docs build: no new errors or warnings, and the same count as before the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A URL whose path carries no recognised extension is now read as an archive and the bytes decide, rather than being rejected on path shape. That shape is what most portals actually hand out: ArcGIS Hub serves shapefiles from a path ending in "data", with the format in the query string. Rejecting it turned away the exact host class the pre-implementation survey identified as the one that matters -- so the requirement to reject any path not ending in .zip or .shp was wrong, and following it verbatim was my mistake. A path naming a different format outright -- .geojson, .kml, .csv, .tif and similar -- is still refused before a request goes out, since that is a mistake worth catching early rather than a download endpoint. URL problems are also no longer classified as fetch failures. They reported the fetch stage, so an unsupported path produced the convert-to-GeoJSON remedy and a retry button: both misleading, because the host was never contacted and re-running an unsupported URL fails identically forever. They now carry their own stage and kind, which withholds both. The not-an-archive message covers both ways it now happens, since an unrecognised path gives no hint which: a host returning an error page with a success status, or a URL pointing at an unzipped shapefile that should have been given as its .shp. Found by testing against a real ArcGIS Hub URL. I had verified that URL's CORS headers and content type but never run it through the validator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two independent defects, both of which produced readable-looking attribute values that were wrong rather than any error. The parser strips field padding with String.prototype.trim(), and NUL is not whitespace in JavaScript. A .dbf that pads its character fields with NUL rather than spaces -- Natural Earth's do -- yielded values carrying their padding, which renders as a run of tofu boxes. An all-padding field was a truthy string of NULs instead of null, so an empty value arrived as content that popups drew and style rules matched. A NUL-padded numeric field failed `+value` and read as null with nothing reported. The parser also defaults to windows-1252 regardless of what the source says it wrote, so a UTF-8 .dbf turned every non-ASCII name into mojibake. The .cpg component that names the encoding was never fetched. Both are handled at the bytes, before the parser sees them: .cpg joins the component set (so the archive and sibling paths both pick it up), NUL padding in the record region is rewritten to spaces on a copy, and the encoding is taken from the .cpg when it names something usable. With no .cpg the record region is sniffed for UTF-8 -- self-validating, so an invalid sequence is proof it is not -- and failing that the parser's own default stands, leaving a file with no encoding information decoding exactly as before. The header is left alone in both cases: a DBF declares its own length at offset 8, everything before it is binary, and everything after it is text for every field type this parser reads. Fixture-backed, with the fixture's own defect asserted so the tests cannot pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the last uncovered lines in controllers.py and zarr_utils.py, both of which reached main without tests. controllers.py: the 500 path where an unexpected conversion failure is logged rather than forwarded, zarr_meta's missing-src rejection, and its two error exits -- 502 when the store cannot be opened, 400 when the metadata read rejects it. The split matters and is now asserted: the open failure's message is swallowed because it can name internal hosts, while the metadata error is forwarded because it describes something the author chose and can act on. zarr_utils.py: open_store (previously always mocked) is now exercised against a stubbed FsspecStore -- that it opens read-only, that it retries a transient failure, and that a persistent one surfaces as StoreOpenError specifically, since the API layer maps that to 502 rather than 400. read_cog gets the same treatment plus its lru_cache, which is what stops a COG's several range reads from each re-opening the store. read_metadata gains the explicit-variable branch, its unknown-variable and missing-transform rejections, and the unsupported-ndim path through _grid_dims. parse_byte_range gains the non-numeric-bound cases that reach its ValueError handler. Verified by mutation rather than by coverage alone: nine of ten seeded defects in these paths are caught. The tenth is the `n <= 0` guard in parse_byte_range, which no input can distinguish -- with a non-positive suffix length `start` lands at or past `total`, which the bounds check below already rejects. The test pins the observable contract instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lifecycle gaps Applied from a multi-agent review of the shapefile branch. Every item below was either reproduced empirically or confirmed by an independent validator before being changed. The 25 MB ceiling did not bind. It was charged against the size each member's local header declares, and fflate's inflater ignores that number entirely -- so a member that under-declared expanded without limit. A 41 KB archive with four bytes edited materialized 40 MB while the budget observed 111. The declared size is now a fast path that refuses an oversized member before inflating it, then is reconciled against the bytes that actually arrive, so a lying header buys nothing. cache.js's "already under the ceiling by construction" comment depended on this holding. Components were classified by extension alone. A shapefile zipped with macOS Finder ships "__MACOSX/._basins.shp", which counted as a second shapefile and got the archive rejected as ambiguous -- telling the author to point at a single shapefile, which is what they had done. AppleDouble twins and directory entries are now skipped, and parts are selected by the .shp's own directory and stem, so an unrelated .dbf elsewhere in the archive can no longer become the attribute table (previously whichever .dbf appeared last simply won). Adding .cpg to the fetched component set widened an existing defect into a likely one: only 404 counted as absence, and an S3 bucket without ListBucket returns 403 for a missing object -- so a layer that worked before would fail over a file it never needed, and most shapefiles ship no .cpg. Any client error on .cpg or .shx is now absence. .prj and .dbf stay strict, because treating a transient 403 on those as absence would draw features in the wrong place with no error at all. deriveSiblingUrls lower-cased every extension, and the .shp is fetched from that table too -- so ".SHP" 404'd its own required component on any case-sensitive host. Siblings now follow the case the author wrote, and the component they named is requested at exactly the URL they gave. The test asserting the lower-casing described it as normalization; it is not normalizable, so the test was wrong rather than the behavior. The loader had no notion of which invocation was current, which produced three failures at once. Aborting stopped the fetch but not the CPU-bound parse, so a discarded run finished and wrote under its layer name -- and status is keyed by name, so it erased a live layer's error and left a blank layer reporting nothing. Retry is never disabled while a load runs and refresh() exists to re-invoke the loader, so two runs overlapped and the second orphaned the first's controller. And nothing caught a throw: OpenLayers calls the loader without a catch of its own, so a rejected dynamic import (a stale tab after a deploy) left the layer on "loading" forever with no error and no retry. A per-invocation currency check now makes a superseded run silent, reset() aborts before restarting, and an escaping throw is reported as a retryable failure. Removed the Shapefile arms of getStyleFields/getLayerAttributes and getShapefileLayerAttributes: both panes short-circuit on shapefileDiscovery and return before reaching them, so the path was unreachable -- and its six direct unit tests reported confidence in code production never ran. Replaced with assertions on the short-circuit itself, which is what was actually untested. AttributesPane's effect omitted shapefileDiscovery from its deps, so fields read after that tab had been visited never arrived. StylePane already listed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nsfer failures The four follow-ups from the review's remaining queue. Load listeners are now detached whenever a load is aborted. The loader already declines to report anything once superseded, which closed one half of the ghost -write path; this closes the other, so a source that outlives its layer has nothing listening to it. It matters because status is kept per layer *name* and a rebuilt layer reuses the name, which is what let a dead source report under a live layer's. Preservation now compares the whole source object rather than only its url. `projection` is the only way to place a shapefile that carries no .prj, and matching on url alone meant editing it preserved the layer and re-read nothing -- the author changed the field, saved, and the map did not move. Comparing the source also covers whatever props it gains next, and the component cache keeps the resulting rebuild cheap when only a non-url prop changed. Two failures that surface at the parse stage are really about the transfer: a portal answering with an HTML error page under a success status, and an archive whose bytes stopped arriving partway through. Both are transient conditions of the host or the connection -- the most retryable things that can happen -- and classifying them by the stage they were noticed at reported them as the author's file being wrong and withheld the retry that would have fixed them. A truncated archive also had the same reason as "this is not a zip at all", so it could not be worded differently; it now has its own. The editor's field read had no AbortSignal at all, so closing the editor or retyping the url left a multi-megabyte fetch running with its result still bound for state, and whichever of two overlapping reads settled last won regardless of which url it was for. Each read now owns a controller, supersedes the previous one, aborts on unmount, and checks it is still current after both the fetch and the parse. A thrown failure is reported rather than leaving the pane on "loading" with its read button disabled. Each fix was mutation-checked. The first attempt at the parse-stage supersede test passed with the guard removed -- its superseded read was parked in the fetch, so the earlier guard caught it and the later one was never exercised. The test now supersedes a read that has cleared the fetch and is sitting in the parse, which is the only way to reach it. The equivalent guard in ModuleLoader was already covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…emand Three cleanups surfaced by the review, none of them defects. The .shx is the record index and nothing reads it -- the parser is handed the .shp and .dbf and walks them sequentially. Fetching it cost a round-trip on every unzipped shapefile, and in the archive path it was inflated into memory and charged against the 25 MB ceiling, which at four bytes per record is real headroom on a large file. Dropped from the component set; the archive path simply stops extracting it. LayerConfigurationBuilder refused four source types the layer editor offers. Shapefile, GeoTIFF, Zarr and Static Image all reached the frontend's source registry without being added to the Python builder, so a plugin author following the path the docs call recommended got a ValueError naming a list the type was missing from. All four are now registered with the layer class getLayerType writes, plus their source properties, so a plugin-built layer matches what the editor produces. Hand-building the raw dict always worked, which is why nothing caught it. proj4, wkt-parser and the definition table cost ~150 KiB in the main bundle plus a quadratic registration pass, and most dashboards have no layer needing any of it. Nothing imports the module statically now. `isNativelyResolvable` moved to its own dependency-free module, since the answer is a property of the code and needed neither proj4 nor the table -- that static import was the whole reason the machinery reached the bundle. The map awaits the module only when a layer could carry or name a non-native CRS, and the shapefile pipeline loads it inside its own async read alongside the parser's dynamic imports. The trigger is deliberately broad: a source reading its CRS out of its own data -- GeoTIFF or Zarr from the file, a shapefile from its .prj -- has no code in its config to inspect, so the type alone requires it. Awaited before any layer is constructed, because importing the module is what registers the table codes and a layer that merely names one needs the definition on hand when OpenLayers resolves it. Verified safe to defer: the view projection is never a table code -- it starts at EPSG:3857 and the auto-fit adopts only natively-resolvable codes -- so nothing resolves a projection before that point. Measured: the feature's cost to main.js drops from +156 KiB to +24 KiB, with 131 KiB moved into chunks that load only when something needs them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e editor Three drifts, all introduced by this branch and all in docs/source/plugins.rst, which is the plugin-authoring page rather than the GUI one and so was missed. The builder's supported-source list named nine types. It now accepts thirteen -- Shapefile, GeoTIFF, Zarr and Static Image were added to valid_sources in this branch, so the page understated what a plugin author can build. clickTolerance and snapToFeatures each enumerate the sources they apply to, and both omitted Shapefile. That was a direct contradiction: layer_tab.rst was updated to say a Shapefile layer supports both, so the same product documented opposite answers on two pages depending on whether the reader came from the editor or from the plugin API. Separately, source_tab.rst claimed an oversized source is "refused before it is expanded". That was true when written and is now only half true: a member declaring an oversized size is still refused before inflating, but one that under-declares is caught as the extra bytes arrive -- which is the whole point of the accounting fix, since the declared size cannot be trusted as a bound. The sentence now says both, because a doc promising the stronger guarantee is how the original defect went unnoticed. Sphinx warning count unchanged from main at 12; the remaining plugins.rst errors are pre-existing title-style and duplicate-target issues at lines 52-849. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Shapefile map layer source
Adds Shapefile as a map layer source, read and rendered entirely in the browser. No backend involvement, no upload — a layer stores a public
http(s)URL and the file stays wherever it already lives.Branch:
feat/shapefile-map-layer-source→main17 commits · 68 files · +7208 / −66
What an author gets
Pick Shapefile in the layer editor's Source tab and paste a URL. Three forms are accepted:
.zip).shpof an unzipped set — the.dbf,.prjand.cpgsiblings are derived and fetched from the same path.zipat all — ArcGIS Hub serves shapefiles from URLs ending in something like/downloads/data?format=shp, and that is the shape most authors actually haveThen Read shapefile fields to pull the
.dbffield names. From there styling, popups, attribute variables and snapping all behave exactly as they do for a GeoJSON layer — the shapefile becomes a plainVectorLayer, so it inherits the existing dispatch rather than getting a parallel implementation.Coordinate reference comes from the file's own
.prj. If it has none, the projection field accepts a code (EPSG:5070) or a full WKT/proj4 definition. If neither is present the layer says its coordinates cannot be placed rather than guessing.Deliberate non-goals
access-control-allow-originat all and cannot be used. The message says so and names the alternative (convert to GeoJSON) rather than leaving the author with a cause and no move.How it is put together
Six new modules under
reactapp/components/map/shapefile/, split along the axes that actually change independently:siblings.jsacquire.jsunzip.jsattributes.js.dbftext decoding and de-paddingindex.jscache.jsPlus
projections.js(a CRS registry for codes a layer names but carries no definition for),projectionCodes.js,layerStatus.js(load-state vocabulary shared with the plugin-layer path), andshapefileDiscovery.js(the editor's author-triggered field read).Acquisition returns raw buffers and knows nothing about what a shapefile means. That split is deliberate: the parser choice was the largest named risk, and the byte accounting and cancellation contract survive a parser swap intact.
Parser choice
shapefile@0.6.6, chosen over the better-known alternative because of ring nesting. The format encodes a polygon's interior rings by winding direction with no parent pointer, so the ring order a parser produces is the only record of which ring is a hole — and a parser that re-derives containment wrongly draws a basin's holes as filled polygons on top of it. Verified against pyshp-generated fixtures using pyshp's independent nesting implementation as the oracle: a holed polygon reads as one Polygon with a hole, a multipart record as one MultiPolygon, coordinate for coordinate.Field discovery is explicit, not automatic
The Style pane's discovery effect re-runs on every source-props change — once per keystroke for a typed URL — and each run here is a multi-megabyte download. So it happens when asked. One read serves both the Style and Attributes tabs. No schema is stored, which keeps the field list true to the source but moves staleness into the rules naming those fields; an upstream rename leaves them matching nothing while the layer still draws, so referenced-but-absent fields are listed explicitly.
New dependencies
Five, all exact-pinned:
shapefile@0.6.6,fflate@0.8.2,proj4@2.21.0,@mapbox/geojson-rewind@0.5.2,wkt-parser@1.5.6.Cost to
main.jsis +24 KiB. It was +156 KiB untilproj4and the projection table were moved behind a dynamic import — the static import of one pure helper (isNativelyResolvable) was dragging the whole machinery into the main bundle. The map now loads it only when a layer could carry or name a non-native CRS. 131 KiB moved into chunks that load on demand.Neither is shapefile-specific. Both are pre-existing defects the shapefile work exposed rather than created, and fixing them changes behavior for existing GeoJSON and feature-service layers:
1. A rule whose condition field is absent no longer matches
Measured operator by operator, comparing old and new against the same inputs:
undefined)!=,notIn— weretrue, nowfalsenull!=,notIn,<,<=— weretrue, nowfalse"",0, or any real valueThe
</<=row is the non-obvious one:null < 5istruein JavaScript becausenullcoerces to0, so a feature with an explicitnullin a field previously satisfied a< 5rule. It no longer does.isNull/isNotNullare untouched — the guard runs after them deliberately, since asking whether an absent field is null has a real answer and a rule styling "no data" depends on it.What to look for: a saved rule using
!=,notIn,<or<=on a field that some features lack. Previously one such rule repainted every feature missing the field; the layer still rendered, so nothing failed and nobody was told.Real values are provably unaffected, which is the property that matters — this function styles every vector layer in the app.
2. An absent attribute variable is cleared rather than left stale
Verified: a plugin arg bound to an attribute variable receives byte-identical output whether the variable was never set or was cleared to
"":So
""is not a new value shape — it is already what every dependent plugin receives on a fresh dashboard load, before any feature is clicked. A plugin that cannot handle an empty arg was already failing then. What changed is that clicking a feature which lacks the bound field now converges on that same already-reachable state, instead of retaining the previously clicked feature's value.The old behavior was the worse of the two and silent: every dependent visualization kept rendering the previous feature's data with no indication it was stale. This is the only path in the map that propagated a wrong value off the map.
Presence is tested rather than truthiness, so a real
0,""orfalseis carried through instead of being dropped — and the field-vs-alias fallback is now nullish-based for the same reason (previously a field value of0would lose to its alias).Unchanged: the literal string
"Null"is still treated as absent, as it was before.Both changes are intended and both are covered, including a regression test asserting present-value comparisons are untouched. They are listed here because they are the parts of this MR that touch layers it is not about.
Also fixed in passing: the reprojection sweep now runs on every path that replaces the map view, not only the raster auto-fit path added in #174. Features already on the map are baked into the outgoing projection's numbers and go invisible — not erroring — if the view moves without a sweep. Shapefile is the slowest-loading vector source in the app and therefore the most exposed.
Testing
controllers.pyandzarr_utils.pybrought to 100% line coverage (pre-existing gaps, test-only — no behavior change)main)mainFixtures are real shapefile bytes written by pyshp rather than hand-crafted, because the behavior under test is how a parser interprets the format's own conventions. The encoding fixture asserts the defect it covers is present in the fixture itself, so those tests cannot pass vacuously.
Reviewed, then hardened
The branch went through a multi-agent review (11 reviewers, 44 raw findings, 13 validated by independent verification, 1 refuted). Everything validated at P1/P2 was fixed in the three
fix(review)commits. Highlights, each reproduced before being fixed:__MACOSX/._basins.shp, which counted as a second shapefile; the archive was refused as ambiguous, telling the author to point at a single shapefile — which they had. Parts are now selected by the.shp's own directory and stem, which also stops an unrelated.dbfelsewhere in the archive from silently becoming the attribute table.try/catchof its own, so a rejected dynamic import — a stale tab after a deploy — left the layer on "loading" forever with no error and no retry.Four tests were found to encode the defects they covered (asserting the misclassification, describing the extension lower-casing as "normalization"). Those were corrected rather than the behavior. Each fix was mutation-checked; one mutation survived and exposed a genuine hole in a new test, which was then closed.
Also cleaned up:
LayerConfigurationBuilderrefused four source types the editor offers (Shapefile, GeoTIFF, Zarr, Static Image) — all four reached the frontend registry without being added to the Python builder, so a plugin author following the documented path got aValueError. Fixed for all four, not just the new one.Not yet verified
No projected CRS has been rendered in a browser. All manual testing used EPSG:4326 sources.
projections.jsexists entirely for projected coordinate systems — proj4 registration, the control-point probe, synthetic WKT codes, extent application — and has thorough unit tests but no end-to-end validation. Worth loading one EPSG:5070 or UTM shapefile before merge.Related: the dashboard corpus has exactly one live layer depending on the registry (a WMS requesting EPSG:5041) and it has not been opened since these changes.
Follow-ups (not blocking)
Map.jscrossed 1000 lines — auseShapefileLayershook would separate the shapefile lifecycle from the generic reconciliation effect.dbfUTF-8 sniff concludes UTF-8 from a single coincidental multi-byte sequence.prj-less shapefile cannot show its fieldsDocs
docs/source/maps/source_tab.rst,layer_tab.rst,style_tab.rst— the source type, its two URL forms, the download-endpoint case, attribute-text encoding, the field read, and the size limit.