Skip to content

fix(micropub): wire up q=geo, fix lint, add tests and changeset - #373

Closed
davidwkeith wants to merge 2 commits into
mainfrom
claude/pr-372-merge-ready-v0c139
Closed

fix(micropub): wire up q=geo, fix lint, add tests and changeset#373
davidwkeith wants to merge 2 commits into
mainfrom
claude/pr-372-merge-ready-v0c139

Conversation

@davidwkeith

Copy link
Copy Markdown
Owner

Summary

This supersedes #372 (feat/micropub-q-geo), carrying its q=geo
Location/Venue commit forward and fixing everything the review on that PR
flagged so it's actually mergeable:

  • Wired up q=geo: handleQuery never had a q === "geo" branch, so
    every real request 400'd with unsupported query even though it was
    advertised in q=config. Added handleGeoQuery, routed it, and fixed the
    q=config advertisement to use a venuesEnabled() gate (mirrors
    contactsEnabled) instead of the previous mis-indented if (config.venues).
  • Fixed CI lint failures: the "./venues.js" import in config.ts had an
    unnecessary escape (".\/venues.js"), and venues.ts used three
    @typescript-eslint/no-explicit-any casts to smuggle a distance field
    through sort/filter — replaced with a properly typed
    { row, distance }/{ distance, venue } shape.
  • Fixed the duplicated union type: venues?: MicropubVenueStore | MicropubVenueStore
    MicropubVenueStore | MicropubVenueStoreProvider, mirroring the existing
    Contacts provider pattern (normalizeVenueStore, MicropubVenueStoreProvider).
  • Fixed the longitude bounding-box prefilter: it used a flat /111000
    for both latitude and longitude, which narrows incorrectly at higher
    latitudes (a degree of longitude shrinks by cos(latitude)) and could
    silently exclude in-radius venues before the exact haversine filter ran.
    Now divides by 111000 * cos(latitude) (guarded near the poles).
  • Fixed the hardcoded venue url: was https://example.com/venues/${id}
    unconditionally; createMicropubVenueStore now takes a { baseUrl } config
    argument and builds the canonical URL from it.
  • Fixed the sort tie-breaker: was alphabetic by name; the spec calls for
    the canonical venue url ascending as the deterministic tie-breaker.
  • Added colocated tests: venues.test.ts covers Geo URI/parameter
    validation (all the malformed/out-of-range/combined-form cases the spec
    lists), the cos(latitude) prefilter fix, distance/url ordering and
    pagination, and the missing-D1-binding startup failure. index.test.ts
    gets an end-to-end suite exercising q=geo through the real handler:
    disabled-by-default non-advertisement + 400, an authorized request
    returning nearby venues, and a malformed-query 400.
  • Added a changeset (.changeset/micropub-q-geo.md, @dwk/micropub minor).
  • Clarified in the README that the bundled store's geo suggestion echoes
    the input coordinates rather than performing real reverse-geocoding.

Closes #354.

Packages affected

@dwk/micropub

Checklist

  • Read the relevant spec(s) under spec/packages/ and updated them if
    behaviour changed — no behavioural change to the spec text was needed;
    README clarified re: the reverse-geocoding placeholder.
  • Added/updated colocated tests (src/*.test.ts)
  • Ran the local CI gate: pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm test
  • Added a changeset (pnpm changeset) if this touches a publishable
    package
  • Updated catalog.json / conformance/status.json if this adds a new
    mountable worker or changes conformance status — not applicable, no new
    worker and @dwk/micropub stays prerelease/exempt from the gate.

Generated by Claude Code

davidwkeith and others added 2 commits July 22, 2026 16:50
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
Addresses PR #372 review feedback: q=geo was advertised in q=config but
handler.ts never routed it, so every request 400'd; CI failed lint on an
escaped import path and no-explicit-any casts; the venue store had zero
test coverage; and no changeset was recorded for the @dwk/micropub change.

Also fixes correctness issues flagged in review: the bounding-box prefilter
now corrects longitude by cos(latitude) so it no longer excludes in-radius
venues away from the equator, venue urls are built from a configured
baseUrl instead of a hardcoded example.com placeholder, and the sort
tie-breaker uses the canonical venue url (per spec) instead of name.

Closes #354

@davidwkeith davidwkeith left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review per CONTRIBUTING.md.

Template/title conformance: looks good. PR title (fix(micropub): wire up q=geo, fix lint, add tests and changeset) follows the required Conventional Commits format, the body keeps the Summary / Packages affected / Checklist headings verbatim, checklist items are ticked with a clear one-line reason on the two intentionally-unchecked ones, and a changeset (.changeset/micropub-q-geo.md) is included for the @dwk/micropub minor bump. This is the shape CONTRIBUTING.md §6/§7 asks for.

Two things worth resolving before merge:

  1. Close #372 instead of leaving it open alongside this one. This PR's own description says it "supersedes #372, carrying its q=geo Location/Venue commit forward" — both PRs target main and both close #354. Leaving #372 open invites a redundant merge/conflict; recommend closing it once this one is confirmed good (or vice versa if further review here turns something up).
  2. CI was still in progress at review timebuild-test was in_progress and github-advanced-security shows failure (this failure is consistent across all three currently-open PRs, so it looks like a pre-existing/unrelated repo-wide GHAS configuration issue rather than something introduced here, but worth a quick check before merging that it isn't masking anything real).

No functional issues found in the diff itself — the cos(latitude) prefilter fix, the typed { row, distance } replacement for the as any casts, and the baseUrl-derived venue URL all look correct against the spec description in the PR body.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Closing as redundant with #372, which carries the same fix for the exact issues this PR addresses (q=geo wiring, lint failures, the cos(latitude) bounding-box correction, the url ascending tie-breaker, as any casts, tests, and a changeset). #372's review thread is already resolved and its CI is green. Consolidating there to avoid two competing PRs both closing #354.


Generated by Claude Code

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.

micropub: Micropub extensions roadmap (maturity-grouped)

2 participants