Skip to content

Add Map API and map pages - #693

Open
oliviasculley wants to merge 19 commits into
hackgvl:developfrom
oliviasculley:map-api
Open

Add Map API and map pages#693
oliviasculley wants to merge 19 commits into
hackgvl:developfrom
oliviasculley:map-api

Conversation

@oliviasculley

Copy link
Copy Markdown
Member

No description provided.

@oliviasculley oliviasculley changed the title Add Map API Add Map API and map pages Mar 20, 2026
@oliviasculley
oliviasculley marked this pull request as ready for review March 20, 2026 05:29
@allella

allella commented Apr 28, 2026

Copy link
Copy Markdown
Member

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?

@claude

claude Bot commented May 7, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@oliviasculley

Copy link
Copy Markdown
Member Author

Probably just a temporary Github bug, when I view the list of open PRs, it appears at the bottom (as the oldest open one)

oliviasculley and others added 2 commits July 26, 2026 02:11
# Conflicts:
#	resources/css/app.css
#	resources/views/layouts/app.blade.php
#	resources/views/layouts/top-nav.blade.php

@allella allella left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Comment thread app-modules/api/src/Http/Controllers/EventApiV1Controller.php Outdated
Comment thread app-modules/api/src/Http/Controllers/MapLayersApiV1Controller.php Outdated
'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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@oliviasculley oliviasculley Jul 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to raw_data_link too.
  • API docs: added @responseField descriptions distinguishing geojson_link (the remote/origin source, may be null) from geojson_url (the HackGreenville-hosted endpoint consumers should actually read GeoJSON from).

I stopped short of renaming the underlying geojson_link column, since that ripples into the migration, model, sync service, seeder, and tests. If you'd rather have a more self-documenting name like geojson_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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think renaming the fields to geojson_remote_link and geojson_local_url would be appropriate so the code is easier to read.

Comment thread app/Filament/Resources/MapLayerResource/Pages/ListMapLayers.php
Comment thread app/Services/MapLayerSyncService.php
Comment thread app/Services/MapLayerSyncService.php Outdated
oliviasculley and others added 2 commits July 31, 2026 17:30
- 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>
@oliviasculley

oliviasculley commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough review, @allella! I first brought the branch up to date with the latest develop (resolved the nav/CSS conflicts from the design-system merge), then pushed ccd38ef addressing the inline code feedback:

  • Reusable LIKE macro — extracted the wildcard-escaping into a whereLikeContains() Eloquent Builder macro, now shared by the Events and Map Layers API controllers.
  • Slug lookups — added an exact-match slug filter to the Map Layers API for clean 1-to-1 queries alongside the fuzzy title search.
  • geojson_link vs geojson_url — documented the remote-source vs HG-hosted distinction in the Filament form and the API docs.
  • Sync hardeningJSON_THROW_ON_ERROR on encode, stricter GeoJSON structure validation, and skipping CSV rows with out-of-range coordinates. Added tests for each.

Replied inline on each thread with specifics.

On the bigger-picture items (301-redirecting the old data.openupstate.org preview URLs and SEO, migrating the layer spreadsheets off the old fiscal-sponsor Workspace, pruning stale County/iMAP layers, the Swamp Rabbit Trail path fix, and the separation-of-concerns question of what belongs under the primary HG domain) — I agree these are important but they're each their own effort beyond this PR, which is focused on standing up the Map API + pages so we can start decommissioning the old server. I'd suggest we capture them as tracking issues so they don't get lost. Happy to open those if you'd like.


protected $description = 'Sync map layer GeoJSON files from their remote data sources';

public function handle(MapLayerSyncService $service): int

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An option to throttle the speed of the sync would be helpful, both for the Artisan commands and through Filament.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@allella allella Aug 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread app/Services/MapLayerSyncService.php Outdated
throw new RuntimeException('Invalid GeoJSON: response was not valid JSON.');
}

if ( ! isset($data['type']) || $data['type'] !== 'FeatureCollection') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.");
    }

@oliviasculley oliviasculley Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 9e36492. I pulled in swaggest/json-schema and now validate the assembled GeoJSON against a real JSON Schema before writing the file, replacing the old type === "FeatureCollection" + features-array checks with true structural validation (assertValidGeoJson() in MapLayerSyncService).

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 at app/Services/geojson-schema.json — it covers FeatureCollectionFeature → all seven geometry types, uses only internal $refs (so validation never touches the network), and the compiled Schema is 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 Point coordinates — 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 Feature must carry type, geometry, and properties members (any of which may be null). 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.

oliviasculley and others added 3 commits August 1, 2026 00:01
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants