feat(micropub): implement q=geo Location/Venue extension - #372
Conversation
Implement read-only proximity search for the Location/Venue feature. - Add VenueStore with D1 storage for geospatial venue queries - Implement haversineDistance for proximity calculation - Parse Geo URIs (geo:lat,lon;u=radius) and lat/lon/u parameters - Return venues ordered by distance with pagination - Add venues config option to enable the extension when proposed group is enabled - Update README with usage examples Closes #354
davidwkeith
left a comment
davidwkeith
left a comment
There was a problem hiding this comment.
Thanks for tackling q=geo — the store design (D1 schema, independent from post storage, haversine proximity) is a reasonable shape and matches the spec's intent. However, this isn't mergeable as-is:
- The feature isn't actually wired up.
q=geois advertised inq=configbuthandleQueryhas noq === "geo"branch, so every real request 400s withunsupported query. See inline comment onhandler.ts. - CI is red.
build-testfails atpnpm lint: an unnecessary escape in thevenues.jsimport path inconfig.ts, and three@typescript-eslint/no-explicit-anyerrors invenues.ts. - No tests.
venues.ts(D1 schema, Geo URI parsing, haversine search, pagination) has no colocatedsrc/*.test.tscoverage, which CONTRIBUTING.md requires for every behaviour change. - No changeset for this
@dwk/micropubuser-visible change. - PR body doesn't follow
.github/PULL_REQUEST_TEMPLATE.md— missing thePackages affectedandChecklistsections CONTRIBUTING.md asks PRs to keep verbatim. - A couple of correctness/placeholder issues worth fixing before this ships: the longitude bounding-box prefilter doesn't correct for
cos(latitude)(see inline), and venueurlis hardcoded tohttps://example.com/venues/....
Happy to re-review once q=geo is actually reachable from the handler, lint is clean, and tests + changeset are added.
Generated by Claude Code
Addresses review feedback on the q=geo Location/Venue extension: - Actually route `q=geo` from handleQuery (it was advertised in q=config but never dispatched, so every real request 400ed). - Fix CI lint failures: unnecessary escape in the venues.js import path, duplicated MicropubVenueStore union member, and three `as any` casts replaced with a typed distance-carrying tuple. - Fix the longitude bounding-box prefilter to correct for cos(latitude) convergence, which previously could exclude in-radius venues at higher latitudes before the haversine filter even ran. - Replace the hardcoded example.com venue URL with a real `url` column populated by whatever writes venue rows (the composing app's own venue system), matching "a venue URL is immutable identity" in the spec. - Add colocated unit and HTTP-level test coverage (parsing, validation, radius/bbox correction, pagination, advertisement gating). - Add a changeset and clarify in the README/spec that the `geo` suggestion is a coordinate-echoing placeholder, not real reverse-geocoding.
davidwkeith
left a comment
davidwkeith
left a comment
There was a problem hiding this comment.
Automated review per CONTRIBUTING.md.
Template/title conformance: looks good. PR title (feat(micropub): implement q=geo Location/Venue extension) follows the required Conventional Commits format, the body keeps the Summary / Packages affected / Checklist headings verbatim, and unchecked items carry a one-line reason rather than being deleted — all per CONTRIBUTING.md §6/§7.
One thing to resolve before merge: #373 (fix(micropub): wire up q=geo, fix lint, add tests and changeset) explicitly supersedes this PR — it carries this branch's commit forward plus fixes for exactly the issues a review here would flag (q=geo never actually wired into handleQuery, lint failures, the duplicated MicropubVenueStore | MicropubVenueStore union type, the flat /111000 lat/lon prefilter, the hardcoded example.com venue URL, alphabetic vs. spec-required tie-breaker). Both PRs target main and both close #354, so having them open simultaneously invites a redundant merge or a conflict. Recommend closing this one in favor of #373 (or, if #373 turns out to need more work, closing #373 instead) rather than reviewing/landing both independently.
Generated by Claude Code
|
Closed #373 in favor of this PR — both were independent fixes for the same review feedback, and this one's thread is already resolved with CI green. This PR is the one to merge for #354. Generated by Claude Code |
2 participants
Summary
Implements the proposed, default-off Location/Venue (
q=geo) extension per the design in #359/#366, closing #354's last open roadmap item.q=geois a read-only proximity search over an injected, strongly-consistent venue store (createMicropubVenueStore, D1-backed), independent from post storage. Accepts a Geo URI or discretelat/lon+ optionaluradius, returns venues ordered by great-circle distance withlimit/offsetpagination. Thegeolocation suggestion is currently a placeholder that echoes the query coordinates back — no real reverse-geocoding service is wired in.Follow-up fixes on top of the original commit, addressing review feedback:
q=geois now actually routed fromhandleQuery(previously advertised inq=configbut unreachable — every real request 400ed).as anycasts replaced with a typed distance tuple).cos(latitude), which could otherwise silently exclude in-radius venues at higher latitudes.example.comvenue URL with a realurlcolumn (venue URL is immutable identity, per spec).Packages affected
Checklist
spec/packages/and updated them if behaviour changedsrc/*.test.ts)pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm testpnpm changeset) if this touches a publishable packagecatalog.json/conformance/status.jsonif this adds a new mountable worker or changes conformance status — not applicable, no new worker and no conformance-status change