Add Map API and map pages - #693
Conversation
|
This PR is still on the radar, but I did pull it up today and noticed it's not showing under open PRs, even though the list of PRs shows (3) and then only lists (2) PRs. Is there some mystery status on this PR? |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
Probably just a temporary Github bug, when I view the list of open PRs, it appears at the bottom (as the oldest open one) |
# Conflicts: # resources/css/app.css # resources/views/layouts/app.blade.php # resources/views/layouts/top-nav.blade.php
There was a problem hiding this comment.
This is far from a complete technical or practical review, but I wanted to finally get something for you to chew on after 4 months.
Thanks for the work on this PR. It's a step in the right direction for a nice UI and API for devs, and probably will see more AI agents use in the near term than we ever had directly from humans.
This will be a great help in decommission the old server in the near-term.
The BIGGER questions surround things pondered a couple of years ago that the primary HG domain and app likely don't need to address.
Maintaining Search / Human Traffic
- We'll need to transition the human search engine or bookmark traffic without breaking the SEO or usability of the old map preview URLs that get the bulk of the traffic and practical use.
- This requires maintaining clean, comparable URL on some domain or subdomain to which we can 301 redirect.
- A quick and dirty implementation would be to 301 the old patterns like https://data.openupstate.org/map/preview/electric-vehicle-charging-stations to
https://hackgvl.github.io/open-map-data-multi-layers-demo/?lat=34.844526&lng=-82.401078&zoom=10&maps=electric-vehicle-charging-stations,- The free hosting and low security concerns are nice, but there are many concerns with simply using the Demo page on a Github pages subdomain as an intermediate or long-term solution.
- the long and variable URLs with a bunch of query strings create usability, duplicate content, and SEO problems
- the use of a Github subdomain is not ideal for a lot of reasons
- it's not trivial / ideal to 301 redirect away from Github pages if we started using the domain as the primary and wanted to change to another domain alter
- poor SEO vs a custom domain or subdomain
- confusing / disconnected branding under Github's domain
- dependency on the whims of mega corp
- Hosting the demo map, or some variant, on a dedicated domain would solve some, but not all of those concerns.
- The free hosting and low security concerns are nice, but there are many concerns with simply using the Demo page on a Github pages subdomain as an intermediate or long-term solution.
Pruning / Migrating Layers
- The original map layer spreadsheets are still owned by a Google Workspace account from Code for the Carolinas old fiscal sponsor. That could be deactivated at any moment, so we also need to migrate the spreadsheets we're keeping to HackGreenville's shared Google Drive. I moved coworking and breweries over in the past, so there's already an established pattern for that.
- We increasingly have stale or hardly used map layers beyond what we had in mid-2024 when I checked which layers were actually getting traffic.
- The iMAP project between Prisma / Furman / Greenville County seems to be be dead. The ArcGIS links (ex. for Recycling) and the iMAP application no longer work .
- We have dozens of basically unsupported County / iMap layers with no maintainers or spreadsheet.
- Many / most of them are low / no traffic and/or not something that's aligned enough to be showing directly under the HG domain.
- We could do a campaign to find new maintainers a consider some layers with no maintainer interest for the scrap heap
- Some of the data we have for the Swamp Rabbit Trail is coming through OpenStreetMaps and/or Mike Nice. It would be good to better sync things between our spreadsheets and what's contributed back to OpenStreetMaps as part of their tile data.
- The Swamp Rabbit Trail main path layer is currently wrong. It's showing the same path data as the Spurs and Connectors. We'll need to fix that.
- We should still talk with Bennett and the Bike Walk Greenville folks to coordinate all the things related the layers, API, and storing the syndicated or original data.
Separation of Concerns
- It was convenient to have a dedicated data domain, separate from HG and Code For Greenville, as the catch all for human traffic and developers alike. Though, it was never a well polished publish facing site or a well used developer site.
- Publicizing the technical stuff fits the hackgreenville.com domain, given HG Labs manages the project. However, I don't see that we'd want to cram 50 random and/or stale map layers into the SEO and structure of the primary HG domain.
| 'center_latitude' => (float) $this->resource->center_latitude, | ||
| 'center_longitude' => (float) $this->resource->center_longitude, | ||
| 'zoom_level' => $this->resource->zoom_level, | ||
| 'geojson_link' => $this->resource->geojson_link, |
There was a problem hiding this comment.
I see geojson_url generates to a link to the GeoJSON file within the HG domain.
geojson_link appears to be the remote source for syndicated GeoJSON, like with the County data or Mike's Swamp Rabbit resources that aren't part of a spreadsheet.
We could include the word remote, origin, or some such to distinguish url vs link and document this distinction in the API docs and Filament form since it's likely to always be a point of confusion even with a more verbose variable naming.
There was a problem hiding this comment.
Agreed this is a perennial point of confusion. In ccd38ef I documented the distinction in both places you called out:
- Filament: relabeled the field to "GeoJSON Source Link (remote)" with helper text spelling out that it's the upstream/origin source Sync pulls from — explicitly not the HG-hosted
geojson_url— and added helper text toraw_data_linktoo.- API docs: added
@responseFielddescriptions distinguishinggeojson_link(the remote/origin source, may be null) fromgeojson_url(the HackGreenville-hosted endpoint consumers should actually read GeoJSON from).I stopped short of renaming the underlying
geojson_linkcolumn, since that ripples into the migration, model, sync service, seeder, and tests. If you'd rather have a more self-documenting name likegeojson_source_url/geojson_remote_link, I'm glad to do that rename in a follow-up — just wanted to keep this PR's surface area contained.
There was a problem hiding this comment.
Yes, I think renaming the fields to geojson_remote_link and geojson_local_url would be appropriate so the code is easier to read.
- Extract the LIKE-wildcard escaping into a reusable `whereLikeContains` Eloquent Builder macro and use it across the Events and Map Layers API controllers (removes duplicated escape logic). - Add an exact-match `slug` filter to the Map Layers API for 1-to-1 lookups alongside the existing partial `title` search. - Clarify the geojson_link (remote source) vs geojson_url (HG-hosted) split via Filament helper text and Scribe @responseField docs. - Harden MapLayerSyncService: encode with JSON_THROW_ON_ERROR, validate the GeoJSON features array, and skip CSV rows with out-of-range coordinates. - Regenerate API docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
|
||
| protected $description = 'Sync map layer GeoJSON files from their remote data sources'; | ||
|
|
||
| public function handle(MapLayerSyncService $service): int |
There was a problem hiding this comment.
An option to throttle the speed of the sync would be helpful, both for the Artisan commands and through Filament.
There was a problem hiding this comment.
I think you can also sync individual map layers, so I don't think we'll necessarily be running the main sync all the time. I do worry about the complexity of adding this feature in, but it might not be that much of an issue
There was a problem hiding this comment.
I gathered from the Artisan sync all command, and the .geojson files in the Git repo, that there was a thought to push changes regularly into the Git repo through a regular job.
I commented earlier that pushing server-based .geojson files / changesets into the repo feels odd. Doing away with a scheduled sync would reduce the original concern that we could trigger a throttle on a remote / origin if we went unconstrained on 40 or 50 network requests in sequence.
I don't know that it's "right", but we did add a sleep(1) to a Meetup prune command (app-modules/event-importer/src/Console/Commands/PruneMissingEventsCommand.php) in the past.
That was probably to avoid a full-throttle rate limiting as Meetup is a bit touchy even at our historical hourly ping.
Sleep adds its own failure points on remote network or script timeouts, so it's perhaps not the best way to do a basic throttle.
Open to ideas on things that don't require background jobs, but still protect against the potential that we over do a sync all and piss off an origin.
There was a problem hiding this comment.
It's probably worth some consideration about if we need these GeoJSON files in the Git repo as part of this app.
Most / all of these are pretty ephemeral and based on remote sources. The files can still live locally, but having them in Git may not be worth the overhead.
There was a problem hiding this comment.
I think I wanted to keep them here as a backup solution, but you're right that they're probably going to be needed to be kept up to date after that point. Maybe it wouldn't be worth it, but I don't trust them only living in one place like Google sheets
There was a problem hiding this comment.
I like backups of backups, but typically binary files and large documents that nobody is likely to ever actually look at for a diffs fall into the scope of dedicated data / file backup jobs.
I get it that these are reasonably small files / changes, but we're talking about a mix of randomly or scheduled .geojson syncs to the server's file system. That would mean we'd be pushing git changes from an ephemeral Railway server's file system back to a git repo, which is the opposite direction of how things usually flow. It would add some weirdness with the server needing its own own SSH key and passphrase key chain in order to commit synced changes to its own code base.
The HG site's copy will effectively be a backup of any origin copies, so that's a loose 2nd copy /
backup.
Cloudflare will be in front of all of this, so that's a short-term loose / 3rd copy.
Railway has backups on the database, so we could push things into the DB to be captured by existing DB backups and/or do additional offsite backups of bits of the server that aren't part of a repo of DB.
We could consider capturing these and other binary files that are outside of the repo and DB as part of an additional scheduled offsite backup going to a bucket or similar dedicated backup system.
| throw new RuntimeException('Invalid GeoJSON: response was not valid JSON.'); | ||
| } | ||
|
|
||
| if ( ! isset($data['type']) || $data['type'] !== 'FeatureCollection') { |
There was a problem hiding this comment.
Given that FeatureCollection isn't checking for true validation, perhaps we could use a lightweight PHP approach with a generic JSON Schema validator like swaggest/json-schema along with the official GeoJSON schema file.
Very rough example below.
try {
// Load the official GeoJSON Schema definition
$schemaJson = file_get_contents('https://geojson.org');
$schema = Schema::import(json_decode($schemaJson));
// Validate your input array or object
$schema->in(json_decode(json_encode($value)));
} catch (\Exception $e) {
$fail("The {$attribute} must be a structurally valid GeoJSON object.");
}
There was a problem hiding this comment.
Implemented in 9e36492. I pulled in
swaggest/json-schemaand now validate the assembled GeoJSON against a real JSON Schema before writing the file, replacing the oldtype === "FeatureCollection"+ features-array checks with true structural validation (assertValidGeoJson()inMapLayerSyncService).A few deliberate departures from the rough sketch:
- Bundled the schema instead of fetching it at runtime.
file_get_contents('https://geojson.org')returns the marketing homepage rather than a schema, and either way I didn't want a sync to depend on a live network call to a third party. I committed a self-contained RFC 7946 schema atapp/Services/geojson-schema.json— it coversFeatureCollection→Feature→ all seven geometry types, uses only internal$refs (so validation never touches the network), and the compiledSchemais cached per process.- Decode to objects, not associative arrays. This was the subtle one:
json_decode($json, true)collapses an empty JSON object like"properties": {}into[], which then fails structural validation and would corrupt the stored file. Both the remote-GeoJSON and CSV-built paths now produce object-based structures so{}round-trips correctly. There's a regression test (test_sync_preserves_empty_properties_as_json_object) locking this in.- On failure the layer's sync fails with a clear
Invalid GeoJSON: …message (surfaced in the Filament notification) rather than silently writing a bad file.Added tests for a malformed-geometry rejection (non-numeric
Pointcoordinates — exactly the case the old check waved through) and the empty-properties round-trip. Full suite is green.One heads-up: the schema follows the spec strictly, so a
Featuremust carrytype,geometry, andpropertiesmembers (any of which may benull). If any of the live County/ArcGIS sources emit looser structures we'll now see those sync attempts fail loudly — which I think is the desired signal, but easy to relax per-field if a real source needs it.
Replace the lightweight `type === FeatureCollection` / features-array checks
in MapLayerSyncService with true structural validation using
swaggest/json-schema and a bundled, self-contained RFC 7946 GeoJSON schema
(all internal $refs, so validation never touches the network).
- Decode remote GeoJSON to objects so empty JSON objects (e.g. `properties: {}`)
survive instead of collapsing to `[]` and breaking both validation and the
stored file.
- Build CSV-derived GeoJSON as objects for the same reason.
- Cache the compiled schema per process.
- Add tests for malformed geometry rejection and empty-properties round-trip.
Addresses PR hackgvl#693 review feedback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No description provided.