fix(trails): response.url crash + MCP tool deduplication#2362
Conversation
- alltrails-preview: fall back to parsed.href when response.url is empty string (occurs in test environments and some CF Worker fetch contexts) - mcp/trails.ts: remove duplicate search_trails/get_trail/get_trail_geometry registrations left from merge; fix preview_alltrails_url endpoint path from /alltrails/preview to /trails/alltrails-preview
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
Coverage Report for API Unit Tests Coverage (./packages/api)
File CoverageNo changed files found. |
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
There was a problem hiding this comment.
Pull request overview
Follow-up fixes to the trails + MCP integration after the OSM trail POC merge, addressing an AllTrails preview response edge case and removing duplicated MCP tool registrations.
Changes:
- Prevent
POST /api/trails/alltrails-previewfrom returning an emptyurlby falling back to the already-validatedparsed.hrefwhenresponse.urlis empty. - Remove duplicate MCP tool registrations for trails tools and keep a single canonical registration set.
- Update the MCP
preview_alltrails_urltool to call the correct API route (/trails/alltrails-preview).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/mcp/src/tools/trails.ts | Removes duplicated tool registrations and fixes the preview tool’s route path. |
| packages/api/src/routes/trails/index.ts | Fixes AllTrails preview response to avoid empty url values from synthetic Response objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…clean up tests
- search_trails returns { trails, hasMore } instead of bare array (006)
LIMIT n+1 trick — caller can page with offset until hasMore is false
- remove POST /trails/alltrails-preview; /alltrails/preview (alltrails.ts)
is the canonical endpoint (015); MCP tool updated to match
- drop dead alltrails-preview test block from trails.test.ts — covered by
alltrails.test.ts (012); remove unused afterEach/vi imports
- fix misleading test name: stitching does not write back to DB (014)
fix(trails): response.url crash + MCP tool deduplication
Summary
Follow-up fixes for issues found during OSM trail POC review after #2306 merged.
POST /api/trails/alltrails-previewreturned 502 in all test cases becauseresponse.urlis an empty string when theResponseis constructed directly (test mocks, some CF Worker fetch contexts). Now falls back toparsed.hrefwhich is already validated as an alltrails.com URL.trails.tshadsearch_trails,get_trail, andget_trail_geometryeach registered twice (once from the OSM DB version, once from the Overpass merge artefact). Removed the duplicates, keeping the cleaner OSM DB parameter names. Also fixedpreview_alltrails_urlendpoint path (/alltrails/preview→/trails/alltrails-preview).What was already fine (no changes needed)
packages/osm-db/drizzle/0001_osm_schema.sqlpackages/api/drizzle/0037_trips_trail_osm_id.sqlpackages/api/drizzle.config.tsTest plan
POST /api/trails/alltrails-previewhappy-path tests pass (previously returned 502)preview_alltrails_urlMCP tool calls the correct route🤖 Generated with Claude Code